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

Implement Happy Eyeballs for TCP #2394

Closed
Tracked by #2523
sukunrt opened this issue Jun 27, 2023 · 3 comments · Fixed by #2573
Closed
Tracked by #2523

Implement Happy Eyeballs for TCP #2394

sukunrt opened this issue Jun 27, 2023 · 3 comments · Fixed by #2573
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@sukunrt
Copy link
Member

sukunrt commented Jun 27, 2023

We currently do not do any prioritisation among TCP addresses when dialing. This is because TCP connection establishment takes too long(3*RTT) and the delay required between dials for effective prioritisation would be roughly 750ms - 1s. This delay is too high in case we fail to establish a connection with the first tcp address we try.

We can get around this if we setup a system where the worker loop is notified when a TCP connection is established and security negotiation is in progress.

@sukunrt sukunrt added the kind/enhancement A net-new feature or improvement to an existing feature label Jun 27, 2023
@marten-seemann
Copy link
Contributor

Thank you for opening an issue on this @sukunrt!

What about changing the Dial function on the Transport from

Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (CapableConn, error)

to

Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) chan DialResult

type DialResult struct {
     Kind DialResultKind // an enum with DialFailed, DialSucceeded and DialProgressed
     Error error
     Connection CapableConn
}

DialProgressed (suggestions for a better name welcome) would be (optionally) emitted by transports that want to notify that the first packet from the server has been received. This is most useful for TCP, but we can also use this for WebSocket and WebTransport, and even for QUIC at some point (QUIC servers might perform a Retry). Since this is an optional event, we can do it for TCP first, and deal with the other transports later.

This is a pretty big change in our Transport API, but since dials are all handled by the swarm, this shouldn't cause to much breakage in the ecosystem.

@sukunrt
Copy link
Member Author

sukunrt commented Jun 27, 2023

I like your suggestion.
I prefer this to having the Transport API take a channel like

Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID, updates chan DialResult) 

This variant has the problem that if the user provides a channel with low capacity it'll block the security upgrade. So it's better to let transport return a channel.

@sukunrt
Copy link
Member Author

sukunrt commented Jul 9, 2023

Summarising discussions with @marten-seemann on this

There are two approaches we can take to allow for updates

1. Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) chan DialResult
2. Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID, chan DialUpdate)

Both of them need an extra goroutine per Dial to handle returned values from the Transport. This is because we do checks on the returned value here

As an alternative we can do

     DialWithUpdates(ctx context.Context, raddr ma.Multiaddr, p peer.ID, chan DialUpdate) (tpt.CapableConn, error) 

where we are only notified of DialProgressed event on the channel and everything else stays the same. This can work but it is a hack just to ensure minimal changes to the swarm code.

I propose we do

  Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID, chan DialUpdate)

And move all post processing which requires a separate go routine outside of swarm.dialAddr to swarm.addConn.

This variant has the problem that if the user provides a channel with low capacity it'll block the security upgrade. So it's better to let transport return a channel.

The best we can do is document this requirement.

@sukunrt sukunrt changed the title Notify dial worker loop when TCP connection is established Implement Happy Eyeballs for TCP Jul 12, 2023
@marten-seemann marten-seemann mentioned this issue Jul 14, 2023
21 tasks
@marten-seemann marten-seemann mentioned this issue Aug 15, 2023
23 tasks
@marten-seemann marten-seemann mentioned this issue Aug 25, 2023
25 tasks
@marten-seemann marten-seemann linked a pull request Oct 17, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
2 participants