QUIC transport + API changes #48
Merged
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
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
This was referenced Jan 4, 2024
merklefruit
approved these changes
Jan 18, 2024
| /// A joinset of authentication tasks. | ||
| pub(super) auth_tasks: JoinSet<Result<AuthResult<T::Io>, PubError>>, | ||
| /// The receiver end of the message broadcast channel. The sender half is stored by [`PubSocket`](super::PubSocket). | ||
| pub(super) from_socket_bcast: broadcast::Receiver<PubMessage>, | ||
| } | ||
|
|
||
| impl<T: ServerTransport> Future for PubDriver<T> { | ||
| impl<T> Future for PubDriver<T> |
There was a problem hiding this comment.
this impl is a lot cleaner, love it 🚀
| // Take the transport here, so we can move it into the backend task | ||
| let mut transport = self.transport.take().unwrap(); | ||
|
|
||
| pub async fn bind(&mut self, addr: SocketAddr) -> Result<(), PubError> { |
There was a problem hiding this comment.
nit: what do you think of following the tokio convention of using the generic ToSocketAddrs trait?
It's the one used in TcpStream::connect:
pub async fn connect<A: ToSocketAddrs>(addr: A) -> io::Result<TcpStream> {
let addrs = to_socket_addrs(addr).await?;
let mut last_err = None;
for addr in addrs {
match TcpStream::connect_addr(addr).await {
Ok(stream) => return Ok(stream),
Err(e) => last_err = Some(e),
}
}
// ...the cool thing is that it automatically resolves dns names which is something we need to do manually right now
There was a problem hiding this comment.
Was also thinking about this, will make issue w.r.t. QoL improvements
Comment on lines
+163
to
+168
| // Some transport implementations (e.g. Quinn) can't dial an unspecified IP address, so replace | ||
| // it with localhost. | ||
| if endpoint.ip().is_unspecified() { | ||
| // TODO: support IPv6 | ||
| endpoint.set_ip(IpAddr::V4(Ipv4Addr::LOCALHOST)); | ||
| } |
There was a problem hiding this comment.
will this work in a real network environment?
Comment on lines
266
to
+267
| layer_stack: Option<Box<dyn Layer<Io> + Send>>, | ||
| should_reconnect: bool, |
There was a problem hiding this comment.
Suggested change
| layer_stack: Option<Box<dyn Layer<Io> + Send>>, | |
| should_reconnect: bool, | |
| layer_stack: Option<Box<dyn Layer<Io> + Send>>, | |
| /// If the session should be reconnected upon an unexpected disconnection | |
| should_reconnect: bool, |
Comment on lines
+78
to
85
| match Pin::new(&mut *self.get_mut().inner).poll_accept(cx) { | ||
| Poll::Ready(mut accept) => match accept.poll_unpin(cx) { | ||
| Poll::Ready(Ok(output)) => Poll::Ready(Ok(output)), | ||
| Poll::Ready(Err(e)) => Poll::Ready(Err(e)), | ||
| Poll::Pending => Poll::Pending, | ||
| }, | ||
| Poll::Pending => Poll::Pending, | ||
| } |
There was a problem hiding this comment.
Suggested change
| match Pin::new(&mut *self.get_mut().inner).poll_accept(cx) { | |
| Poll::Ready(mut accept) => match accept.poll_unpin(cx) { | |
| Poll::Ready(Ok(output)) => Poll::Ready(Ok(output)), | |
| Poll::Ready(Err(e)) => Poll::Ready(Err(e)), | |
| Poll::Pending => Poll::Pending, | |
| }, | |
| Poll::Pending => Poll::Pending, | |
| } | |
| match Pin::new(&mut *self.get_mut().inner).poll_accept(cx) { | |
| Poll::Ready(mut accept) => accept.poll_unpin(cx), | |
| Poll::Pending => Poll::Pending, | |
| } |
| } | ||
|
|
||
| /// Creates a new [`quinn::Endpoint`] with the given configuration and a Tokio runtime. If no `addr` is given, | ||
| /// the endpoint will be bound to the default address. |
There was a problem hiding this comment.
Suggested change
| /// the endpoint will be bound to the default address. | |
| /// the endpoint will be bound to the unspecified address. |
Comment on lines
+220
to
+225
| fn accept(&mut self) -> crate::Acceptor<'_, Self> | ||
| where | ||
| Self: Sized + Unpin, | ||
| { | ||
| Acceptor::new(self) | ||
| } |
There was a problem hiding this comment.
default impl can be omitted (at your preference)
Suggested change
| fn accept(&mut self) -> crate::Acceptor<'_, Self> | |
| where | |
| Self: Sized + Unpin, | |
| { | |
| Acceptor::new(self) | |
| } |
Merge activity
|
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.
refactor(transport): extract TCP transport to module
refactor: rework socket interface with generic transport
wip: quic
Closes #44