multitask quic socket operations #611
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sakridge
commented
Apr 5, 2024
lijunwangs
reviewed
Apr 5, 2024
| @@ -185,8 +224,37 @@ async fn run_server( | |||
| stats.clone(), | |||
| coalesce, | |||
| )); | |||
|
|
|||
| let mut accepts = incoming | |||
There was a problem hiding this comment.
Please add some comments please on what we are trying to achieve.
There was a problem hiding this comment.
Suggested change
| let mut accepts = incoming | |
| // Create the set of accept() futures we're going to listen on for connections. We wrap quinn's | |
| // incoming.accept() so we can store the endpoint index in EndpointAccept::endpoint. When a | |
| // future resolves, we use the index to call incoming[index].accept() again. This way we don't | |
| // have to recreate the whole set of accept() futures at every loop iteration, saving wakeups | |
| // and tokio runtime work. | |
| let mut accepts = incoming |
alessandrod
approved these changes
Apr 5, 2024
| @@ -275,6 +275,9 @@ mod tests { | |||
| ) | |||
| .unwrap(); | |||
|
|
|||
| let response_recv_endpoint = response_recv_endpoints | |||
There was a problem hiding this comment.
Not strictly related to this PR because pre-change the test is doing the same,
but: why is it using one of the server endpoints? Why isn't it creating a new
endpoint? Makes the test confusing - I had to triple check that it's fine to
just pick one from the now multi-endpoints.
Maybe add a comment that explicitly says that it doesn't really matter which
endpoint is used?
| @@ -185,8 +224,37 @@ async fn run_server( | |||
| stats.clone(), | |||
| coalesce, | |||
| )); | |||
|
|
|||
| let mut accepts = incoming | |||
There was a problem hiding this comment.
Suggested change
| let mut accepts = incoming | |
| // Create the set of accept() futures we're going to listen on for connections. We wrap quinn's | |
| // incoming.accept() so we can store the endpoint index in EndpointAccept::endpoint. When a | |
| // future resolves, we use the index to call incoming[index].accept() again. This way we don't | |
| // have to recreate the whole set of accept() futures at every loop iteration, saving wakeups | |
| // and tokio runtime work. | |
| let mut accepts = incoming |
| let timeout_connection = select! { | ||
| ready = accepts.next() => { | ||
| if let Some((connecting, i)) = ready { | ||
| accepts.push( |
There was a problem hiding this comment.
Suggested change
| accepts.push( | |
| // accept() for socket incoming[i] has resolved. Queue another so we keep | |
| // accepting from the socket. | |
| accepts.push( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
quinn uses a single task to do socket operations. not great
Summary of Changes
use SO_REUSEPORT to fanout socket operation to multiple quinn::Endpoints
Fixes #