[network] Makes mesh-dependent network tests asynchronous #1095
Conversation
huitseeker
force-pushed
the
network-races-chunk2
branch
3 times, most recently
from
d47d81f
to
4ffa8f0
Compare
Codecov Report
@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
+ Coverage 53.14% 53.18% +0.03%
==========================================
Files 321 321
Lines 21730 21755 +25
==========================================
+ Hits 11549 11570 +21
- Misses 8614 8618 +4
Partials 1567 1567
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| type TagLessConnManager interface { | ||
| connmgr.ConnManager | ||
| // ProtectPeer increments the stream setup count for the peer.ID | ||
| ProtectPeer(id peer.ID) |
There was a problem hiding this comment.
The ProtectPeer and UnprotectPeer are only called when a one-to-one stream is established between two nodes (only caller are libP2PNode.CreateStream -> tryCreateNewStream()) It is not called when a node connects to the other node as part of the PeerManager.
Hence, when testing nodes in a purely pub-sub connections, the TagLessConnManager observer will not be notified.
Libp2p nodes heartbeat every one second hence 2 seconds guarantee that peers have discovered each other and formed a mesh (their own tests do something similar)
If we want an event driven test and avoid these sleeps we could explore the libp2p EventBus - https://github.com/libp2p/go-libp2p-core/blob/master/event/network.go.
There was a problem hiding this comment.
Our tagless ProtectPeer and UnprotectPeer are indeed not called in pure pub-sub connections, but even if they were, they're not the calls that the TagWatchingConnManager intercepts and builds an Observable on.
What we are capturing here is the calls made by libp2p's tagtracer in the pubsub package. They call the tag-full Protect (and Unprotect), not the tag-less ones.
There was a problem hiding this comment.
ah ok ! got it..thanks
huitseeker
force-pushed
the
network-races-chunk2
branch
from
4ffa8f0
to
cb93248
Compare
huitseeker
force-pushed
the
network-races-chunk2
branch
3 times, most recently
from
7c3987e
to
563b65d
Compare
- updates our tag-less variant of ConnManager to make the interface we're using clear, - make the functions that use it (bootstraplibP2PNode and associated) take the interface - add a specialized ConnManager in testUtils which Protect events are observable, - this allows observing the pubsub network, which emits tagged protect events on mesh connection - return those on libp2p Node createion (testutils' `GenerateIDs` and dependents)
This makes TestMiddlewareTestSuit/TestUnSubscribe asynchronous, eliminating the race condition
huitseeker
force-pushed
the
network-races-chunk2
branch
from
e31d3dc
to
fff589b
Compare
This reverts commit 845a00b. Reasons for reversal: - provided impetus for clearing out races (onflow#1073, onflow#1095) - network tests now pass with race detector on! - but those timings are preposterous
This reverts commit 845a00b. Reasons for reversal: - provided impetus for clearing out races (onflow#1073, onflow#1095) - network tests now pass with race detector on! - but those timings are preposterous
The network tests in
TestMeshNetTestSuiteandTestMiddlewareTestSuit/TestUnsubscribedepend on waiting for mesh formation between libp2p nodes, and usetime.Sleepfor that, and are therefore flaky (see issue).The present PR:
ConnManagerintestUtilsthat captures the taggedProtectevents generated by gossipsub at the establishment of a PubSub mesh, using theobservable.Observableinterface,observable.Observerinstance that waits for the correct number of such events to notice the establishment of the mesh,time.Sleepinstances,Obsoletes #1020
Fixes https://github.com/dapperlabs/flow-go/issues/5710