fix(dot/network): fix data race and add `t.Parallel` to all tests by EclesioMeloJunior · Pull Request #2105 · ChainSafe/gossamer

EclesioMeloJunior · 2021-12-06T21:41:51Z

Changes

Removes all the race conditions at dot/network package

Tests

go test ./dot/network -race -v -short

On apple silicon processors

MallocNanoZone=0 go test ./dot/network -race -v -short

Issues

Closes dot/network: remove race conditions and parellelize tests #2044

Primary Reviewer

@timwu20

…el()

qdm12

An early review, sorry I couldn't hold myself as I got excited with all this test parallelism 💯 😄

dot/network/utils_test.go

dot/network/test_helpers.go

lib/utils/test_utils.go

dot/network/service_test.go

EclesioMeloJunior · 2021-12-07T19:52:23Z

blocked by PR #2106

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

dot/network/host.go

dot/network/service.go

dot/network/service_test.go

dot/network/utils_test.go

dot/peerset/peerset.go

dot/peerset/peerstate.go

internal/log/logger.go

test.log

…ssamer into eclesio/net-race-conditions

dot/network/host.go

codecov · 2022-01-14T20:59:41Z

Codecov Report

Merging #2105 (dd9a674) into development (d2ee47e) will increase coverage by 2.65%.
The diff coverage is 59.05%.

❗ Current head dd9a674 differs from pull request most recent head f1b9a3f. Consider uploading reports for the commit f1b9a3f to get more accurate results

@@               Coverage Diff               @@
##           development    #2105      +/-   ##
===============================================
+ Coverage        57.40%   60.06%   +2.65%     
===============================================
  Files              217      214       -3     
  Lines            28587    27807     -780     
===============================================
+ Hits             16411    16701     +290     
+ Misses           10487     9381    -1106     
- Partials          1689     1725      +36

Flag	Coverage Δ
unit-tests	`60.06% <59.05%> (+2.65%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/network/config.go	`63.01% <ø> (ø)`
lib/utils/test_utils.go	`0.00% <0.00%> (ø)`
dot/peerset/handler.go	`56.88% <63.15%> (-0.73%)`	⬇️
dot/network/service.go	`55.94% <77.77%> (-1.07%)`	⬇️
dot/peerset/peerstate.go	`76.73% <85.71%> (+7.60%)`	⬆️
dot/network/host.go	`65.37% <100.00%> (+1.06%)`	⬆️
dot/network/notifications.go	`69.38% <100.00%> (+5.59%)`	⬆️
dot/network/stream_manager.go	`93.18% <100.00%> (-0.57%)`	⬇️
dot/core/service.go	`0.00% <0.00%> (-75.13%)`	⬇️
dot/config.go	`24.90% <0.00%> (-73.24%)`	⬇️
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ee47e...f1b9a3f. Read the comment docs.

jimjbrettj

A few comments but overall looks good, great work! Network is a good place to remove race conditions so good stuff

dot/network/host_test.go

dot/network/service.go

jimjbrettj · 2022-01-28T19:02:22Z

dot/network/service_test.go

 	}
-	require.Equal(t, 6, len(handler.messages[nodeA.host.id()]))
+
+	time.Sleep(time.Millisecond * 200)


seconding this question

jimjbrettj · 2022-01-28T19:03:39Z

dot/network/stream_manager.go

-	ctx           context.Context
-	streamDataMap *sync.Map //map[string]*streamData
+	ctx                   context.Context
+	streamDataMap         *sync.Map //map[string]*streamData


SYNC.MAP??? Probably out of scope I would think. If we really are anti sync map maybe we should open an issue/epic to remove them from codebase?

jimjbrettj · 2022-01-28T19:10:51Z

dot/peerset/handler.go

+
+		defer h.writersWG.Done()
+
+		select {


I'm a bit confused what the purpose of these two selects are, why cant they just be one? Would you mind explaining just for my own learning?

basically, the first one prevents later goroutines to write on the already closed channel, the second is when the close case is triggered while we are blocked trying to write in a full channel, but this code is a way to gracefully close a channel that is being used by other goroutines (since closing a channel while writing causes race condition).

Ooofff this bad. You don't have any guarantee h.closeCh won't get closed between the two select blocks (and hence closing h.actionQueue). Please change somehow.

jimjbrettj · 2022-01-28T19:13:04Z

dot/peerset/peerstate.go

-	ps.mu.Lock()
-	defer ps.mu.Unlock()
-
 	n, err := ps.getNode(pid)


we don't need to lock before this call?

The PeersState causes race condition into network pkg because the getNode function access the ps.nodes map at the same time addReputation updates the map. I added Mutex to PeersState struct so getNode uses the Lock/Unlock to lock while reading and after getNode executes addReputation locks to write. It is not possible locking after getNode since getNode will starve waiting the unlock that will never be called, makes sense? 😅

However the previous implementation is not good so I updated to use a RWMutex instead and remove the getNode call from addReputation which uses the Lock/Unlock and now getNode uses RLock/RUnlock, basically getNode will blocks if another goroutine calls the addReputation first

qdm12 · 2022-01-28T20:32:21Z

dot/network/config.go

 	Telemetry telemetry.Client
-	Metrics   metrics.IntervalConfig
+
+	MessageCacheTTL time.Duration


nit I think you can have this unexported

qdm12 · 2022-01-28T20:32:56Z

dot/network/helpers_test.go

 )

 type testStreamHandler struct {
+	sync.Mutex


nit Maybe name the mutex so we know what it is protecting? Is it the messages map? Or something else?

qdm12 · 2022-01-28T20:33:04Z

dot/network/helpers_test.go

 }

 func (s *testStreamHandler) handleMessage(stream libp2pnetwork.Stream, msg Message) error {
+


Suggested change

qdm12 · 2022-01-28T20:33:35Z

dot/network/helpers_test.go

 	for {
 		tot, err := readStream(stream, msgBytes)
 		if errors.Is(err, io.EOF) {
+			s.eofCh <- struct{}{}


Can you close it instead? I doubt you would get EOF twice right?

qdm12 · 2022-01-28T20:35:00Z

dot/network/host_test.go

-	expected := []ma.Multiaddr{
-		mustNewMultiAddr(fmt.Sprintf("/ip4/127.0.0.1/tcp/%d", port)),
-		mustNewMultiAddr(fmt.Sprintf("/ip4/10.0.5.2/tcp/%d", port)),
+		require.True(t, multiAddrRegex.MatchString(addr.String()))


nit you can use require.Regexp(t, "myregex", "mystring")

qdm12 · 2022-01-28T20:56:24Z

dot/peerset/handler.go

+		h.writerWGMutex.Lock()
+		h.writersWG.Add(1)
+		h.writerWGMutex.Unlock()


You should add to the writes wait group before launching the goroutine. And also the mutex protecting the waitgroup looks sketchy, is it really needed?

qdm12 · 2022-01-28T20:57:45Z

dot/peerset/handler.go

+
+		defer h.writersWG.Done()
+
+		select {


Ooofff this bad. You don't have any guarantee h.closeCh won't get closed between the two select blocks (and hence closing h.actionQueue). Please change somehow.

qdm12 · 2022-01-28T20:58:12Z

dot/peerset/handler.go

 func (h *Handler) SortedPeers(setIdx int) chan peer.IDSlice {
 	resultPeersCh := make(chan peer.IDSlice)
-	h.actionQueue <- action{
+	h.setActionQueue(action{


perhaps rename to addToActionQueue?

qdm12 · 2022-01-28T20:58:33Z

dot/peerset/handler.go

+		h.writerWGMutex.Lock()
+		h.writersWG.Wait()
+		h.writerWGMutex.Unlock()


A mutex for the wait group looks very strange, try to find another way 🤔 ?

qdm12 · 2022-01-28T20:59:24Z

dot/peerset/peerstate.go


 // peers return the list of all the peers we know of.
 func (ps *PeersState) peers() []peer.ID {
+	ps.mu.RLock()


perhaps name mu with a name so we know what it's protecting? I guess peerStateMutex??

timwu20 · 2022-01-31T15:03:24Z

dot/network/host_test.go

@@ -273,24 +271,32 @@ func TestExistingStream(t *testing.T) {
 	require.NoError(t, err)

 	time.Sleep(TestMessageTimeout)


can we refactor this test to not use sleeps anymore? Now that you have testStreamHandler, you can write messages into an internal channel from testStreamHandler.handleMessage and just listen on that channel.

timwu20 · 2022-01-31T15:03:57Z

dot/network/host_test.go

 	require.NoError(t, err)

-	time.Sleep(TestBackoffTimeout)
+	timeout := time.NewTimer(TestBackoffTimeout)


I don't mind this naming.

timwu20 · 2022-01-31T15:05:25Z

dot/network/service_test.go

@@ -237,35 +237,36 @@ func TestBroadcastMessages(t *testing.T) {
 	// simulate message sent from core service
 	nodeA.GossipMessage(anounceMessage)
 	time.Sleep(time.Second * 2)


can we remove sleep?

timwu20 · 2022-01-31T15:15:05Z

dot/network/service_test.go

+	// All 5 message will be sent since cache is disabled.
 	for i := 0; i < 5; i++ {
 		nodeA.GossipMessage(announceMessage)
 		time.Sleep(time.Millisecond * 10)


why do we need to sleep here?

timwu20 · 2022-01-31T15:16:04Z

dot/network/service_test.go

+	addrInfoB := nodeB.host.addrInfo()
+	err := nodeA.host.connect(addrInfoB)
+	// retry connect if "failed to dial" error
+	if failedToDial(err) {


why would this error out on connect? I see this multiple times in this file. I would assume this thing should be ready? Otherwise, we should listen on some sort of internal ready channel to know that it's ready.

dot/network/service.go

timwu20 · 2022-03-16T18:00:49Z

@EclesioMeloJunior can you resolve the conflicts?

EclesioMeloJunior · 2022-03-16T19:49:08Z

@EclesioMeloJunior can you resolve the conflicts?

Of course, once I finish PR #2267 which this one depends on

timwu20 · 2022-10-21T20:22:05Z

@EclesioMeloJunior can you change this to draft if you don't intend to merge this.

EclesioMeloJunior added 2 commits December 6, 2021 09:54

fix: remove race conditions on dot/network

d05e39e

chore: use available ports queue and update all test to call t.Parall…

0611f92

…el()

qdm12 reviewed Dec 7, 2021

View reviewed changes

EclesioMeloJunior added 5 commits December 7, 2021 09:10

chore: resolve data races on dot/network

1e03e39

chore: add locks to global logger New

1009d6c

chore: resolve data race with stop channel to peerset

dd4f9c7

chore: data race at peerstate nodes

f6d51f0

chore: data race on cfg.SlotDuration

589c4a9

EclesioMeloJunior mentioned this pull request Dec 7, 2021

fix(dot/telemetry): remove singleton and data races #2106

Closed

chore: remove unused comment

04b417a

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

qdm12 reviewed Dec 8, 2021

View reviewed changes

EclesioMeloJunior added 3 commits December 9, 2021 09:10

chore: remove test log

9fa0bf0

Merge branch 'eclesio/net-race-conditions' of github.com:ChainSafe/go…

034c4db

…ssamer into eclesio/net-race-conditions

Merge branch 'development' into eclesio/net-race-conditions

b4ee352

EclesioMeloJunior mentioned this pull request Dec 13, 2021

chore(dot): transform telemetry into an injectable interface #2113

Merged

Merge branch 'development' into eclesio/net-race-conditions

9bbc83b

qdm12 reviewed Jan 5, 2022

View reviewed changes

dot/network/host.go Outdated Show resolved Hide resolved

dot/network/host.go Outdated Show resolved Hide resolved

EclesioMeloJunior added 2 commits January 6, 2022 08:49

Merge branch 'development' into eclesio/net-race-conditions

894e218

Merge branch 'development' into eclesio/net-race-conditions

8dc1d05

EclesioMeloJunior mentioned this pull request Jan 14, 2022

fix(dot/telemetry): telemetry hashes to be in the hexadecimal format #2194

Merged

EclesioMeloJunior added 2 commits January 14, 2022 16:29

Merge branch 'development' into eclesio/net-race-conditions

ae518e8

chore: remove unused stopCh

56b20e0

EclesioMeloJunior added 6 commits January 14, 2022 17:08

chore: resolve stream handler race condition

5879434

chore: resolve race at TestExistingStream

03f77aa

chore: resolve race at (*PeersState).getNode()

bbaa958

chore: fix data race at TestStreamCloseEOF

0518139

chore: add MessageCacheTTL at node config

ec8dece

chore: split tests to avoid data race

59cc124

chore: improving code

74df15f

EclesioMeloJunior changed the title ~~fix(dot/network): fix data race and add t.Parallel~~ fix(dot/network): fix data race and use t.Parallel in all tests Jan 26, 2022

chore: remove availablePortQueue

3a8a47c

EclesioMeloJunior requested review from qdm12 and timwu20 January 27, 2022 23:24

EclesioMeloJunior added 5 commits January 28, 2022 11:48

chore: fix problems with port number 0

37046e1

chore: use map to avoid inner loops

1fc3b73

Merge branch 'development' into eclesio/net-race-conditions

3bf902a

chore: remove unneeded code

290fd1b

chore: use availablePort(t) for tests

fa16436

EclesioMeloJunior changed the title ~~fix(dot/network): fix data race and use t.Parallel in all tests~~ fix(dot/network): fix data race and add t.Parallel in all tests Jan 28, 2022

EclesioMeloJunior changed the title ~~fix(dot/network): fix data race and add t.Parallel in all tests~~ fix(dot/network): fix data race and add t.Parallel to all tests Jan 28, 2022

chore: replace available port by 0 at rpc integration tests

6ca2cfd

jimjbrettj approved these changes Jan 28, 2022

View reviewed changes

chore: using rwmutex in addPeerReputation and getNode

dd9a674

qdm12 reviewed Jan 28, 2022

View reviewed changes

chore: address comments

c84d611

timwu20 requested changes Jan 31, 2022

View reviewed changes

Merge branch 'development' into eclesio/net-race-conditions

c46a26d

EclesioMeloJunior mentioned this pull request Feb 3, 2022

fix(dot/peerset): remove race conditions from peerset package #2267

Merged

qdm12 reviewed Feb 10, 2022

View reviewed changes

dot/network/service.go Outdated Show resolved Hide resolved

kishansagathiya mentioned this pull request Mar 17, 2022

feat(lib/grandpa): ensure catch-up logic works #2152

Closed

1 task

EclesioMeloJunior added 3 commits May 30, 2022 15:14

Merge branch 'development' into eclesio/net-race-conditions

c8f7deb

chore: fix lint

5e70577

chore: just fall through the select case

f1b9a3f

EclesioMeloJunior marked this pull request as draft October 21, 2022 21:30

danforbes closed this Aug 18, 2023

		}

		func (s *testStreamHandler) handleMessage(stream libp2pnetwork.Stream, msg Message) error {

		@@ -273,24 +271,32 @@ func TestExistingStream(t *testing.T) {
		require.NoError(t, err)

		time.Sleep(TestMessageTimeout)

Conversation

EclesioMeloJunior commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Issues

Primary Reviewer

Uh oh!

qdm12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EclesioMeloJunior commented Dec 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jimjbrettj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EclesioMeloJunior commented Dec 6, 2021 •

edited

Loading

codecov bot commented Jan 14, 2022 •

edited

Loading

EclesioMeloJunior commented Mar 16, 2022 •

edited

Loading