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

Cleanup goroutine structure of autonat #41

Merged
merged 23 commits into from
Mar 11, 2020

Conversation

willscott
Copy link
Contributor

waiting on libp2p/go-libp2p#747 for detecting local address changes

@willscott
Copy link
Contributor Author

pausing here for sanity check review of structure. If this organization seems plausible, there are a couple additional tests that should go with it.

client.go Outdated Show resolved Hide resolved
* incoming connections post a channel event - fix libp2p#40
* inbound connections reduce the frequency of probes - address libp2p#35

waiting on libp2p/go-libp2p#747 for detecting local address changes
autonat.go Outdated Show resolved Hide resolved
@willscott willscott mentioned this pull request Mar 6, 2020
@willscott
Copy link
Contributor Author

@aarshkshah1992 I merged your #51 into this branch in fe1f5df. A sanity check review before I merge this would be great :)

@aarshkshah1992
Copy link
Collaborator

@willscott Will take a look first thing tomorrow.

autonat.go Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
notify.go Outdated Show resolved Hide resolved
autonat.go Show resolved Hide resolved
autonat.go Show resolved Hide resolved
@willscott willscott requested a review from aarshkshah1992 March 9, 2020 19:54
Copy link
Collaborator

@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.

LGTM modulo one unresolved design decision.

autonat.go Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
autonat.go Outdated Show resolved Hide resolved
notify.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

LGTM modulo couple of nits.

delay = AutoNATRetryInterval
}
timer := time.NewTimer(delay)
timerRunning := true
Copy link
Collaborator

Choose a reason for hiding this comment

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

defer timer.Stop ?

private int
public int
pubaddr ma.Multiaddr
if timerRunning && !timer.Stop() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the timer docs, just this should be fine:

	if !t.Stop() {
 		<-t.C
 	}

We can then get rid of the timerRunning variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for timerRunning is that if the timer fires above, then the channel will have already been drained. t.Stop() on a fired timer will return false, but in that code path we don't want to attempt to drain t.C again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this.

@willscott willscott merged commit 4cd130c into libp2p:master Mar 11, 2020
willscott pushed a commit that referenced this pull request Mar 13, 2020
…ibp2p/go-libp2p-0.5.2

Bump github.com/libp2p/go-libp2p from 0.5.1 to 0.5.2
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.

Don't spawn and sleep a goroutine every time we receive a connection.
5 participants