Support for Hole punching #233
Conversation
aarshkshah1992
changed the title
aarshkshah1992
requested review from
Stebalien,
marten-seemann,
willscott and
vyzo
swarm.go
Outdated
| if cLen >= bestLen { | ||
| best = c | ||
| bestLen = cLen | ||
| } |
There was a problem hiding this comment.
Probably easier:
if isDirectConn(best) && !isDirectConn(c) {
continue
}
if cLen >= bestLen {
best = c
bestLen = cLen
}There was a problem hiding this comment.
Have implemented something similar now.
|
cc @vyzo |
swarm.go
Outdated
| if cLen >= bestLen { | ||
| best = c | ||
| bestLen = cLen | ||
| } |
There was a problem hiding this comment.
Actually this is not correct: we always want to replace a relayed connection with a direct connection, regardless of how many streams it has.
There was a problem hiding this comment.
Ah, missed this. Have fixed it.
swarm_dial.go
Outdated
| if forceDirect { | ||
| goodAddrs = addrutil.FilterAddrs(goodAddrs, manet.IsPublicAddr, s.nonProxyAddr) | ||
| } |
There was a problem hiding this comment.
hrm... yeah, this could be problematic here. If we are behind the same NAT we want the private dial.
Let's not filter only public addrs?
There was a problem hiding this comment.
@vyzo Have addressed your review. Once you approve, will propagate released deps and merge.
swarm_dial.go
Outdated
| if forceDirect { | ||
| goodAddrs = addrutil.FilterAddrs(goodAddrs, manet.IsPublicAddr, s.nonProxyAddr) | ||
| } |
swarm.go
Outdated
| if cLen >= bestLen { | ||
| best = c | ||
| bestLen = cLen | ||
| } |
There was a problem hiding this comment.
Ah, missed this. Have fixed it.
swarm.go
Outdated
| if cLen >= bestLen { | ||
| best = c | ||
| bestLen = cLen | ||
| } |
There was a problem hiding this comment.
Have implemented something similar now.
swarm_dial.go
Outdated
| @@ -516,7 +542,6 @@ dialLoop: | |||
| remoteAddrChan = nil | |||
| continue | |||
| } | |||
|
|
|||
swarm_dial.go
Outdated
| // skip addresses in back-off | ||
| if !s.backf.Backoff(p, a) { | ||
| nonBackoff = true | ||
| // TODO How do we do backoff for forcedirect |
There was a problem hiding this comment.
Honestly, I think we should just NOT do backoffs for forced direct conns at all. We only do this during hole punching anyways and a hole punching attempt can fail for a variety of reasons other than the address being bad.
@Stebalien Any thoughts ?
There was a problem hiding this comment.
Ok, fair enough, I am fine with it.
There was a problem hiding this comment.
I've removed this TODO.
swarm_dial.go
Outdated
| if conn != nil { | ||
| if forceDirect { | ||
| if isDirectConn(conn) { | ||
| log.Debugf("ignoring dial error because we have a connection: %s", err) |
There was a problem hiding this comment.
this log could be better, perhaps ".. because we already have a direct connection ..."?
|
@vyzo Green tick please !!!!!!!!!!!!!!!!!!! |
swarm.go
Outdated
| s.conns.RLock() | ||
| defer s.conns.RUnlock() | ||
|
|
||
| var best *Conn | ||
| bestLen := 0 | ||
| for _, c := range s.conns.m[p] { | ||
| for i := range s.conns.m[p] { |
There was a problem hiding this comment.
so we don't use the i anywhere, why not write this as for _, c := ... as it was before?
There was a problem hiding this comment.
Given that we are looping over a collection of reference types(pointers), it's easier to reason about the code if we use the reference semantic form of the range loop and avoid copies:
https://www.ardanlabs.com/blog/2017/06/for-range-semantics.html
There was a problem hiding this comment.
this is nonsense; we don't use the i and we iterate over connections.
We should use the builtin syntax for that, which doesn't turn one line into two.
| ds.dialsLk.Lock() | ||
| defer ds.dialsLk.Unlock() | ||
|
|
||
| actd, ok := ds.dials[p] | ||
| if !ok { | ||
| adctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
This needs to use the background context. Otherwise, if the first dialer cancels, we'll cancel the overall dial.
There was a problem hiding this comment.
This is also why the tests are failing.
| t := s.TransportForDialing(addr) | ||
| return !t.Proxy() |
There was a problem hiding this comment.
This could panic. I know we don't call it in a way that can panic, but it could still panic and someone else could call it.
For libp2p/go-libp2p#1039.
To implement the hole punching co-ordination as described in libp2p/specs#173, we need to
support forcing a direct connection (if possible) even if a Relayed connection to a peer already exists.
TODO