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

transports/quic: Refactor listener handling #17

Closed
wants to merge 44 commits into from

Conversation

elenaf9
Copy link

@elenaf9 elenaf9 commented Jul 4, 2022

Description

Note: this PR also includes all commits from libp2p#2652. They have been squash-merged into the upstream master already, but I can not merge the latest master here because of the muxer changes. Once the target branchkpp:libp2p-quic is updated to those changes (at least to the point where libp2p#2707 was merged upstream) I can rebase onto the latest master.

Discussed in libp2p#2289 (comment).
This PR adapts the quic transport to the upstream listener changes of libp2p#2652. Additionally, it now allows the quic transport to listen on multiple addresses / endpoints. The changes include:

  • QuicTransport now owns and drives multiple endpoints. An endpoint is created each time Transport::listen_on is called.
  • When dialing a remote, a random existing listening endpoint is used. If none exists a new one is created that only supports outbound connections (i.e. server config is None). If then a new listening endpoint is created any following dials will use the new endpoint, but existing connections are not migrated.

This PR is WIP and still needs some cleaning. Opened now just to avoid duplicate work.
@kpp the PR only affects QuicTransport, but does not change anything major in Endpoint. Connection and QuicMuxer are not touched at all, so it should not conflict with your work on the muxer changes.

Open Questions

  • If a first listening endpoint is created and we already have an endpoint used for dialing (without server capabilities enabled): should we, instead of creating a new endpoint, enable server capabilities on the old endpoint and rebind it to the new listening address? The benefit of this would be that the remote peers could then update their address information once they notice the address change. For later dial attempts they would then know the correct address to dial.
  • Rare case: if the user starts listening on a socket address that we already randomly chose for an existing dialer-endpoint, what should happen?

elenaf9 and others added 30 commits May 16, 2022 02:45
Remove `Transport::Listener: Stream`. Instead require the Transport
itself to implement a stream-like API with `Transport::poll`.

In case of multiple listeners, transports are now required to handle
the multiple listener streams themselves internally.
Remove ListenersStream, instead poll the boxed transport directly
in the `Swarm` for `TransportEvent`s (which replace the former
`ListenersEvent`s).
Add new struct `GenTcpTransport` as wrapper for `GenTcpConfig` to manage
multiple listener streams.
This is essentially the old ListenerStream logic from swarm/connection.
Adapt majority of helper transports to the new Transport trait.
For most transports this just removes the extra *Listener type and
instead implements that logic in `Transport::poll`.

To adapt the `Boxed` transport the restriction had to be added that
transport is `Unpin`.
TODO: check if we can solve polling `Boxed` without the inner Transport
being unpin.
With `Transport` becoming non-Clone and having `&mut` self receivers,
the `Sync` requirement no longer makes any sense and we can thus
remove it.
With PR libp2p#2667 the `Sync` trait bound for transport::Boxed is removed.
If a tcp transport should be polled as a stream we can now do this via
`TcpTransport::new(..)::boxed` and do not need a separate impl of
`Stream` for it.
Return a ListenerId in Transport::listen_on instead of getting it
injected from the outside.
Namespace ListenerIds with the Transport's TypeId to avoid clashing IDs
when transport generate their ListenerIds independenlty.
@elenaf9 elenaf9 marked this pull request as draft July 4, 2022 16:55
@kpp
Copy link
Owner

kpp commented Jul 5, 2022

Thank you. I will carefully review your PR.

@mxinden
Copy link

mxinden commented Jul 6, 2022

Rare case: if the user starts listening on a socket address that we already randomly chose for an existing dialer-endpoint, what should happen?

I would be fine with not handling this case, that is erroring on the listen_on call, forcing the user to handle the case.

I am guessing re-using the existing dialer-endpoint is complicated, especially when handling listen requests across interfaces (i.e. 0.0.0.0) as we would need to do an additional listen_on call for the remaining interfaces with the specified port, right?

@mxinden
Copy link

mxinden commented Jul 6, 2022

  • If a first listening endpoint is created and we already have an endpoint used for dialing (without server capabilities enabled): should we, instead of creating a new endpoint, enable server capabilities on the old endpoint and rebind it to the new listening address? The benefit of this would be that the remote peers could then update their address information once they notice the address change. For later dial attempts they would then know the correct address to dial.

This sounds very neat to me, especially the part where the remote is notified of the address change. I am fine with this landing in a later iteration in case it is a big undertaking.

Also, something to consider when making a call here: Does the go-libp2p QUIC transport support the case? //CC @marten-seemann

Copy link

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Unsolicited review. Feel free to ignore.

keypair: &libp2p_core::identity::Keypair,
multiaddr: Multiaddr,
) -> Result<Self, tls::ConfigError> {
pub fn new(keypair: &libp2p_core::identity::Keypair) -> Result<Self, tls::ConfigError> {
Copy link

Choose a reason for hiding this comment

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

🚀

Great to see this simplification.

transports/quic/src/endpoint.rs Show resolved Hide resolved
}*/
/// Builds a new `Endpoint` that only supports outbound connections.
pub fn new_dialer(config: Config) -> Result<Arc<Endpoint>, transport::Error> {
let socket_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 0);
Copy link

Choose a reason for hiding this comment

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

Just double checking here, does the use of IPv4Addr::LOCALHOST allow to dial endpoints not colocated on the same host?

transports/quic/src/endpoint.rs Show resolved Hide resolved
}
} else {
// Pick a random listener to use for dialing.
// TODO: Prefer listeners with same IP version.
Copy link

Choose a reason for hiding this comment

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

listen_addr: ma,
})
} else {
self.poll_if_addr(cx)
Copy link
Owner

Choose a reason for hiding this comment

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

Are we okay with an infinite recursion in case there are infinite many IPv6 events when we are bound to IPv4?

Copy link
Author

Choose a reason for hiding this comment

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

That can only happen if the IfWatcher will always return Poll::Ready(ipv6_event) if we poll it, which would be a problematic bug either way, no?

I'd like to catch the case here that the IfWatcher first reports an IPv6 address and directly afterwards an Ipv4 one. In that situation we can not just return Poll::Pending here but instead have to solve it by either a) wrapping the whole block in a loop and continue if an Ipv6 address is reported, or b) do this recursion. In both cases the behaviour that you described would be a problem. No sure if this can be solved any other way?

});

let server_config = new_connections.map(|c| (c, config.server_config.clone()));

// TODO: just for testing, do proper task spawning
async_global_executor::spawn(background_task(
Copy link
Owner

Choose a reason for hiding this comment

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

I believe this one can be polled directly, right?

@kpp
Copy link
Owner

kpp commented Jul 7, 2022

but I can not merge the latest master here because of the muxer changes. Once the target branchkpp:libp2p-quic is updated to those changes (at least to the point where

@elenaf9 See #18

@elenaf9
Copy link
Author

elenaf9 commented Jul 10, 2022

Sorry for the force-push, rebasing got a bit messy. Closing this PR instead in favor of #19, which includes the changes of #18 and merges the latest master.

@elenaf9 elenaf9 closed this Jul 10, 2022
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.

4 participants