-
Notifications
You must be signed in to change notification settings - Fork 151
feat(s2n-quic-dc): Reuse handshakes across application processes #2854
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
Conversation
| let acceptor = udp::Acceptor::new( | ||
| id, | ||
| socket, | ||
| &self.stream_sender, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Start struct could be fully reused in the manager once we can move this stream sender for UDP to a behavior struct like for TCP. (manager server does not have a stream sender)
| return Err(Some(error)).into(); | ||
| } | ||
| WorkerState::Sending { future, event_data } => match future.as_mut().poll(cx) { | ||
| Poll::Ready(res) => match res { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried reusing the poll_send function here, but wasn't able to resolve the borrowing issues when calling the function with the future from the WorkerState, not sure if there is a way around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not worried about it for the initial PR, but we can take a look together at doing so.
| let recv_buffer: either::Either<_, Channel> = Either::A(recv_buffer); | ||
| let sub = self.env.subscriber(); | ||
|
|
||
| let map = Map::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a placeholder map here to send to accept_stream, afaict it is only used for handling unexpected packets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want to avoid creating a new map for each stream -- that's pretty expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we create a single placeholder map and reuse that for all streams?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly? I'm not sure what we're using the map for. Maybe a placeholder would work, or we can edit the signatures to have Option<&Map> if that is better/easier.
| Poll::Ready(Ok(ControlFlow::Continue(()))) | ||
| } | ||
| Err(err) => { | ||
| debug!("Error sending message to socket {:?}", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to close the stream here, something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that seems right.
f29e579 to
1efc867
Compare
1efc867 to
ed64761
Compare
dc/s2n-quic-dc/events/acceptor.rs
Outdated
| /// The ID of the stream | ||
| stream_id: u64, | ||
|
|
||
| /// The amount of time the TCP stream spent in the queue before being enqueued |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which queue is this? I suspect this is time from TCP accept() to UDS sendmsg()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the "fresh" queue. Can change the comment to "before being sent over UDS"
dc/s2n-quic-dc/events/acceptor.rs
Outdated
| sojourn_time: core::time::Duration, | ||
|
|
||
| /// The number of times the Unix domain socket was blocked on send | ||
| #[measure("blocked_count")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for sending this particular stream? It might make sense to switch this (or add) a #[counter(...)] as well.
Measure will emit histograms.
| #[allow(dead_code)] | ||
| env: Environment<S>, | ||
| #[allow(dead_code)] | ||
| acceptor_rt: runtime::Shared<S>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have the allow(dead_code) here? Are these only kept for a Drop impl or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed those fields here, but I'm not sure why we have it in the original server in the first place. fmt/clippy/build do not seem to complain without it.
| return Err(Some(error)).into(); | ||
| } | ||
| WorkerState::Sending { future, event_data } => match future.as_mut().poll(cx) { | ||
| Poll::Ready(res) => match res { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not worried about it for the initial PR, but we can take a look together at doing so.
| let recv_buffer: either::Either<_, Channel> = Either::A(recv_buffer); | ||
| let sub = self.env.subscriber(); | ||
|
|
||
| let map = Map::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want to avoid creating a new map for each stream -- that's pretty expensive.
Mark-Simulacrum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to fix the packet format (i64 -> u64) but otherwise I think this is ready to land.
| if keys | ||
| .application | ||
| .opener | ||
| .on_decrypt_success(recv_buffer.into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a future PR: we will probably want to replace this with a call to decrypt(...), since we ought to verify that this is actually a valid packet before we mark it as used in the replay protection map.
Release Summary:
Reuse handshakes across application processes to reduce memory utilization.
Resolved issues:
Description of changes:
This PR introduces a manager server and an application server, which are intended to run in separate processes. A client handshakes with and connects to the manager server. The manager server accepts incoming streams and transfers them via Unix domain sockets to the application server. The application server receives these streams and continues communicating with the client. The same handshake can be reused with multiple manager servers.
Call-outs:
Added in comments.
Testing:
Added happy path integration test. The client handshakes once and connects to two manager servers, reusing the same handshake. It then continues communication with separate application servers.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.