Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Support for Hole punching #233

Merged
merged 12 commits into from
Feb 18, 2021
Merged

Support for Hole punching #233

merged 12 commits into from
Feb 18, 2021

Conversation

aarshkshah1992
Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 commented Jan 14, 2021

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

  • Update & propagate deps.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Why do we use the context to store if we want to force a direct dial? This is not the most obvious API, took me a minute to realize what's going on here. Is there a better solution?
If not, can we add an explanation to Swarm.dial why this route was chosen?

swarm.go Outdated
if cLen >= bestLen {
best = c
bestLen = cLen
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably easier:

if isDirectConn(best) && !isDirectConn(c) {
    continue
}
if cLen >= bestLen {
     best = c
     bestLen = cLen
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have implemented something similar now.

swarm.go Show resolved Hide resolved
swarm_dial.go Show resolved Hide resolved
@aarshkshah1992
Copy link
Collaborator Author

cc @vyzo

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

A couple of small but important points that should be addressed.

swarm.go Show resolved Hide resolved
swarm.go Outdated
Comment on lines 413 to 416
if cLen >= bestLen {
best = c
bestLen = cLen
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is not correct: we always want to replace a relayed connection with a direct connection, regardless of how many streams it has.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, missed this. Have fixed it.

swarm_dial.go Outdated
Comment on lines 408 to 410
if forceDirect {
goodAddrs = addrutil.FilterAddrs(goodAddrs, manet.IsPublicAddr, s.nonProxyAddr)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator Author

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@vyzo Have addressed your review. Once you approve, will propagate released deps and merge.

swarm.go Show resolved Hide resolved
swarm_dial.go Outdated
Comment on lines 408 to 410
if forceDirect {
goodAddrs = addrutil.FilterAddrs(goodAddrs, manet.IsPublicAddr, s.nonProxyAddr)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

swarm.go Outdated
Comment on lines 413 to 416
if cLen >= bestLen {
best = c
bestLen = cLen
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, missed this. Have fixed it.

swarm.go Outdated
if cLen >= bestLen {
best = c
bestLen = cLen
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have implemented something similar now.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

looks good overall, a few stylistic comments.

One issue that remains is what do with the backoff -- see the TODO. Shouldn't this be addressed before merge?

swarm_dial.go Outdated
@@ -516,7 +542,6 @@ dialLoop:
remoteAddrChan = nil
continue
}

Copy link
Contributor

Choose a reason for hiding this comment

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

restore this line pls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be addressed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vyzo

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fair enough, I am fine with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed this TODO.

swarm_dial.go Show resolved Hide resolved
swarm_dial.go Outdated
if conn != nil {
if forceDirect {
if isDirectConn(conn) {
log.Debugf("ignoring dial error because we have a connection: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

this log could be better, perhaps ".. because we already have a direct connection ..."?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@aarshkshah1992
Copy link
Collaborator Author

@vyzo Green tick please !!!!!!!!!!!!!!!!!!!

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

one final nit.

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] {
Copy link
Contributor

Choose a reason for hiding this comment

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

so we don't use the i anywhere, why not write this as for _, c := ... as it was before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@vyzo vyzo Feb 18, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@aarshkshah1992 aarshkshah1992 merged commit f82c425 into master Feb 18, 2021
@aarshkshah1992 aarshkshah1992 deleted the feat/hole-punching branch February 18, 2021 09:04
ds.dialsLk.Lock()
defer ds.dialsLk.Unlock()

actd, ok := ds.dials[p]
if !ok {
adctx, cancel := context.WithCancel(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

This needs to use the background context. Otherwise, if the first dialer cancels, we'll cancel the overall dial.

Copy link
Member

Choose a reason for hiding this comment

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

This is also why the tests are failing.

Comment on lines +340 to +341
t := s.TransportForDialing(addr)
return !t.Proxy()
Copy link
Member

Choose a reason for hiding this comment

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

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.

@Stebalien Stebalien mentioned this pull request May 11, 2021
27 tasks
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants