hole punching support

[vyzo]

Needs testing, but feel free to review.

See libp2p/go-libp2p#1048

[aarshkshah1992]

I don't understand the QUIC codebase enough to be able to give this a meaningful review.

Requesting @marten-seemann to please take a look.

[marten-seemann]

Why is this an error?

[vyzo]

So this is the tricky part.

Ideally we would return the connection accepted by listen, but this is problematic:

1. we need to hook into the listen and intercept that
2. the connection would be returned to the swarm twice, which is very undesirable for obvious reasons.
3. We also need to signal to the swarm that we are done trying to connect, and propagate this to the caller.

I don't think it is safe to return nil, nil here to the swarm, as it will probably crash if it doesn't get a connection and no error.

So I couldn't find anything better to do, than just return an error and let the caller (who actually supplied the context option after all) to check for the incoming connection.

Any ideas for improving this situation are welcome!

[vyzo]

Granted, this should probably be a named error (if we decide to keep this logic) so that the caller knows to wait for the incoming connection and not have to guess.

[vyzo]

Actually, now that I think about it, if the incoming connection succeeds the Connect call will not return an error to the caller.
I still think we need to return an error to the swarm (so that it doesn't expect a connection) though.

[marten-seemann]

Can you point me to where this ctx is configured and Dial is called? I tried grepping through the libp2p workspace, but the only occurrence for WithSimultaneousConnect is in go-libp2p-core, so I assume it's only in a PR at the moment?

[vyzo]

It's brand new -- it's gonna come from this pr: libp2p/go-libp2p#1057
I don't think aarsh has added yet, but he will do very soon.

[marten-seemann]

I'm ok with returning a named error here. Hooking into Listen and catching the connection there might be another (more complicated) solution, but I'm not sure if we're going to run into problems if both peers think that they're a client.

[vyzo]

Yeah, I am not sure either but I think that the swarm is not going to like it.
I'll make a named error for this, maybe it should live in core however?

[vyzo]

added named error.

[aarshkshah1992]

@vyzo

Actually, now that I think about it, if the incoming connection succeeds the Connect 
call will not return an error to the caller.

Is there some magic in Swarm that does NOT return an error for the Connect call even if the dial fails but an incoming succeeds ? Please can you point me to it ?

[marten-seemann]

Code LGTM. Thank you @vyzo!

Once we have a test case this should be good to merge.

[vyzo]

what's this stuff aarsh? I am going to drop these commits when we are to merge.

[aarshkshah1992]

@vyzo It's all debugging. We'll drop it all. DW

[marten-seemann]

There's a race condition here: If we receive 2 connections before the other go routine has deleted this entry from the holePunching map, we'll block here until the context is done.

[Stebalien]

1. It's fixed now, right?
2. Why not delete from the hole-punching map when we first access it? That should fix all the problems here, right?

[Stebalien]

Actually, if we one-buffer the channel and atomically delete from the map, we should be able to do everything under the same lock:

        l.transport.holePunchingMx.Lock()
        holePunch, ok := l.transport.holePunching[key]
        if ok {
            holePunch.connCh <- conn
            delete(l.transport.holePunching, key)
        }
        l.transport.holePunchingMx.Unlock()

Probably not necessary, but may make things easier to reason about.

[marten-seemann]

@Stebalien Can I ask you for a review here? I think I fixed a race condition that could lead to a deadlock when we'd two connection from the same address, but I'd like to get a second set of eyes before merging this.

[Stebalien]

Mainly: make the handoff more atomic.

[Stebalien]

1. It's fixed now, right?
2. Why not delete from the hole-punching map when we first access it? That should fix all the problems here, right?

[Stebalien]

Actually, if we one-buffer the channel and atomically delete from the map, we should be able to do everything under the same lock:

        l.transport.holePunchingMx.Lock()
        holePunch, ok := l.transport.holePunching[key]
        if ok {
            holePunch.connCh <- conn
            delete(l.transport.holePunching, key)
        }
        l.transport.holePunchingMx.Unlock()

Probably not necessary, but may make things easier to reason about.

[Stebalien]

(also happy to walk over this in-person as well)

[marten-seemann]

@Stebalien

• fixed the timer
• fixed the race condition when accepting connections (this time for real, hopefully)

[Stebalien]

nit: we can also just use a custom type holePunchKey{addr string, id peer.ID}. Not critical, but that would be slightly easier to reason about.