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

Forget unnecessary metadata on disconnect #93

Closed
Stebalien opened this issue Sep 3, 2019 · 8 comments · Fixed by #184
Closed

Forget unnecessary metadata on disconnect #93

Stebalien opened this issue Sep 3, 2019 · 8 comments · Fixed by #184

Comments

@Stebalien
Copy link
Member

We currently remember:

  • User agents
  • Public keys
  • Supported protocols

Indefinitely. However, we always re-learn these on reconnect and these values can take >100MiB of memory.


One solution to reducing memory usage is to put these on disk but that seems like a waste when we don't even need to remember them. We should consider removing these on disconnect. Alternatively, we should consider periodically sweeping the peerstore, removing these from peers that we haven't seen for a while.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Sep 11, 2019

@Stebalien

I like the idea of proactively removing metadata for peers that disconnect since we WILL get the metadata back when the re-connect. Here's a basic sketch of how we can implement this.

Expose remove on all peer store components

type KeyBook interface {
    Remove(peer.ID) error  
    ...
}

type PeerMetadata interface {
    Remove(peer.ID) error
    ...
}

AddrBook & ProtoBook already expose removal of peer metadata.

type Peerstore interface {
 // calls KeyBook.Remove(), PeerMetadata.Remove() etc etc. 
   RemoveMetadata(peer.ID) error 
   ...
}

Implement all the above methods in go-libp2p-peerstore.

When we create a new libp2p node, register a Notifiee that calls Remove on the Peerstore when a peer disconnects.

func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
   ....
   ....
   h, err := bhost.NewHost(ctx, swrm, ....)
   If error == nil {
      pstoreNotif := Create Notifiee
      swrm.Notify(pstoreNotif)  
   }
   ...
}

Let me know what you think/if there is a better way to do it. Thanks ! :)

@raulk
Copy link
Member

raulk commented Sep 11, 2019

Ideally the swarm would emit an event on the event bus on disconnect, and the peerstore would perform the removal on receiving such events. Putting the burden of cleanup on downstream modules leaks implementation details.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Sep 11, 2019

@raulk I see what you mean. Is the event bus different from the swarm notifications(register a Notifiee) we subscribe to ?

If yes, please can you point me to some code where I can see how the event bus is used ?

Or, do you mean to say that we should pass a swarm instance to the peerstore and let peerstore register a Notifiee?

@raulk
Copy link
Member

raulk commented Sep 11, 2019

@aarshkshah1992 Have a look at how the identify and identify delta protocols deal with local and peer protocol updates: https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/identify/id.go

Search that repo and go-libp2p-core for these keywords: EvtPeerProtocolsUpdated EvtLocalProtocolsUpdated.

@Stebalien
Copy link
Member Author

Ideally the swarm would emit an event on the event bus on disconnect, and the peerstore would perform the removal on receiving such events. Putting the burden of cleanup on downstream modules leaks implementation details.

Importantly, this would help us process these events in order. We really need to be careful about removing information about connected peers.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Sep 12, 2019

Adding some context to @Stebalien 's above comment for posterity.

Currently, the swarm publishes connect/disconnect notifications to all subscribers that subscribe via the Swarm.Notify() method. In addition to burdening the swarm with this responsibility, it also causes subscribers to sometimes receive the connect/disconnect notifications out of order(since the publishing for both events is done on separate threads), leaving the consumers to deal with the race.

The plan is to move to the swarm publishing connect/disconnect events on the 'same emitter' on the host eventbus so that the ordering among them is preserved.

@Stebalien
Copy link
Member Author

Note: Disconnect/Connect events are always sent in-order for a specific connection. The issue is that the peerstore could process a connection close event while the identify service processes an open event (for a new connection).

  1. Peerstore sees no more connections.
  2. Identify gets a new connection.
  3. Identify adds stuff to the peerstore.
  4. Peerstore removes stuff.

We could probably fix this with a simple (striped/per-peer) lock on the peerstore. Then we'd:

Clean:

  1. Lock.
  2. Check if we have no connections. Otherwise, abort.
  3. Remove the metadata.
  4. Unlock.

Put:

  1. Lock.
  2. Check if we have a connection, otherwise abort.
  3. Put.
  4. Unlock.

But this gets a bit nasty...

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Sep 14, 2019

@raulk @Stebalien

Publish connection events to the bus
libp2p/go-libp2p-core#57
libp2p/go-libp2p-swarm#140

Support for removing PubKey
libp2p/go-libp2p-core#63
#94

Please take a look. Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants