Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Endpoint::reject_new_connections #1585

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

ecton
Copy link
Contributor

@ecton ecton commented Jun 6, 2023

Closes #1584

This commit adds a new function that refuses new connections without impacting existing connections. Internally, this just sets the connection limit to 0, which causes incoming connections to be rejected. This is the same approach that was taken in 0.8.5 when Incoming was dropped.

Let me know if I should also add a unit test to the quinn crate. Since it's a simple pass-through, I elected not to initially.

Closes quinn-rs#1584

This commit adds a new function that refuses new connections without
impacting existing connections. Internally, this just sets the
connection limit to 0, which causes incoming connections to be rejected.
This is the same approach that was taken in 0.8.5 when `Incoming` was
dropped.
@Ralith
Copy link
Collaborator

Ralith commented Jun 6, 2023

Thanks for the PR! Have you tried instead using the existing Endpoint::set_server_config interface to accomplish this in your application?

@djc
Copy link
Member

djc commented Jun 6, 2023

@Ralith maybe we can pass a bool to wait_idle() that takes care of the TransportConfig trick? I agree that the existing method is fairly hard to discover and not very intuitive.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an important API for certain graceful shutdown patterns, including it probably makes sense for discoverability.

quinn-proto/src/endpoint.rs Outdated Show resolved Hide resolved
quinn-proto/src/tests/util.rs Outdated Show resolved Hide resolved
quinn/src/endpoint.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented Jun 6, 2023

maybe we can pass a bool to wait_idle()

That doesn't seem very discoverable either, TBH. After discussion I think this is fine.

These changes address the comments on the pull request.
@djc djc merged commit 98f5fe2 into quinn-rs:main Jun 7, 2023
/// zero.
pub fn reject_new_connections(&mut self) {
if let Some(config) = self.server_config.as_mut() {
Arc::make_mut(config).concurrent_connections(0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this me Arc::make_mut and also handle a case where the value is used concurrently more explicitly? I don't think clone-on-write is really what's meant here...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purposes of this code, it seems sufficient to me that self.server_config holds the modified config. Seems to me that we don't really care that Connections are holding on to a "stale" ServerConfig that still allows more concurrent_connections.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread what Arc::make_mut actually does. Yeah, this design is good!

djc pushed a commit that referenced this pull request Jun 13, 2023
This commit adds a new function that refuses new connections without
impacting existing connections. Internally, this just sets the
connection limit to 0, which causes incoming connections to be rejected.
This is the same approach that was taken in 0.8.5 when `Incoming` was
dropped.
@djc djc mentioned this pull request Jun 13, 2023
Ralith pushed a commit that referenced this pull request Jun 13, 2023
This commit adds a new function that refuses new connections without
impacting existing connections. Internally, this just sets the
connection limit to 0, which causes incoming connections to be rejected.
This is the same approach that was taken in 0.8.5 when `Incoming` was
dropped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to reject incoming connections after quinn 0.9?
4 participants