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

Don't spawn and sleep a goroutine every time we receive a connection. #40

Closed
Stebalien opened this issue Feb 11, 2020 · 4 comments · Fixed by #41
Closed

Don't spawn and sleep a goroutine every time we receive a connection. #40

Stebalien opened this issue Feb 11, 2020 · 4 comments · Fixed by #41
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@Stebalien
Copy link
Member

The way we're currently handling connect events (notify.go) is awful.

@Stebalien Stebalien added the kind/enhancement A net-new feature or improvement to an existing feature label Feb 11, 2020
willscott added a commit to willscott/go-libp2p-autonat that referenced this issue Feb 14, 2020
* 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
willscott added a commit to willscott/go-libp2p-autonat that referenced this issue Feb 14, 2020
* 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
willscott added a commit to willscott/go-libp2p-autonat that referenced this issue Feb 17, 2020
* 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
@aarshkshah1992
Copy link
Collaborator

aarshkshah1992 commented Feb 26, 2020

@Stebalien @willscott

Really, we shouldn't need to sleep for Identify to complete(we already have the EvtPeerIdentificationCompleted event on the bus for this) OR cache AutoNAT peer addresses like this at all to protect them from the wrath of disconnections/peerstore expiry. IMO, a rough sketch for doing this would be:

  1. Get [Peerstore] Pin/Unpin interface for peerstore go-libp2p#800 in first.
  2. We now subscribe to the EvtPeerIdentificationCompleted message on the Eventbus & if a peer supports the AutoNAT protocol, note down it's peerId so we can keep pinning it till we get an error while trying to probe it. Once we get a dial error, we remove the peerId from our cache & Unpin it.
  3. We use all peers in the store which support the AutoNAT protocol & have a public address for probing.

@aarshkshah1992
Copy link
Collaborator

@Stebalien

Can we change the title to "Refactor how we manage AutoNAT peers" as if done right, this will also help solve #9 AND #28 .

@jacobheun If Sir Steb agrees, we should make this a feature & add it to Working Kademlia.

@willscott
Copy link
Contributor

After we note down ids's of peers supporting autonat, how do we choose which peer in that list to attempt dialing?

@Stebalien
Copy link
Member Author

Really, we shouldn't need to sleep for Identify to complete(we already have the EvtPeerIdentificationCompleted event on the bus for this)

We should use the identify event. We didn't before because it didn't exist.

Can we change the title to "Refactor how we manage AutoNAT peers" as if done right, this will also help solve #9 AND #28 .

Let's create a new issue with a specific proposal (we can create multiple issues with different proposals). This is issue is just a bug report about a specific issue.


Note: IMO, in the best of all worlds, we wouldn't handle this in autonat at all. We'd just ask the peerstore "who have we seen recently that supports the autonat protocol". But that's further off.

For now, just remembering a set of peers in this service (no need to block on "pins").

willscott added a commit that referenced this issue Mar 11, 2020
* Single goroutine managing autonat-relevent events.
* Watching incoming connections and local interface changes as signals.
* Emit a single 'rechabilitychanged' persistent event

fix #40 
fix #36 
fix #35
fix #34 
fix #11
obsolete #28
obsolete #9 

Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants