Fix PubSub subscribe on connection race condition #809
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #809 +/- ##
===========================================
Coverage ? 83.69%
===========================================
Files ? 82
Lines ? 14907
Branches ? 0
===========================================
Hits ? 12476
Misses ? 2431
Partials ? 0 |
lchenut
approved these changes
Jan 10, 2023
| @@ -382,7 +380,8 @@ method subscribePeer*(p: PubSub, peer: PeerId) {.base, gcsafe.} = | |||
| ## messages | |||
| ## | |||
|
|
|||
| discard p.getOrCreatePeer(peer, p.codecs) | |||
| let pubSubPeer = p.getOrCreatePeer(peer, p.codecs) | |||
There was a problem hiding this comment.
This creates a remote peer pubsubpeer but with the local peer pubsub.codecs and adds the remote peer to the local peer's peers table. When this executes https://github.com/vacp2p/nim-libp2p/blob/master/libp2p/protocols/pubsub/pubsub.nim#L379 the remote peer continues with the wrong codecs, as it's already present in the table.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a subtle race condition that I discovered while working on #807. Since 807 renders the upgrade "faster" (less callback and asyncSpawn etc), it's highlights some bugs, which is cool
I think this race condition can also happen without 807.
1 - A peer connect to us, and opens the pubsub stream very fast, preloaded with the list of topic its subscribed to
2 - We see that we have a topic in common, so we GRAFT him
3 - "sendConn" stream is setup.
Most of the time, 3 happens before 2. But when 2 happens first, the GRAFT is simply swallowed by this:
https://github.com/status-im/nim-libp2p/blob/1711c204eab2596e4bd42b162d630bc262704daf/libp2p/protocols/pubsub/pubsubpeer.nim#L253
and we end up with an assymetric mesh.
This PR simply wait for both direction to be setup before handling the peer messages.
It's a simplistic fix, and doesn't handle more complex cases (ie, the send stream getting closed, but not the receive one)
The better fix would be to get rid of this silent swallow, and make sure it never happens, but that's a much bigger effort