Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eth: fix race condition in eth/sync tests #522

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
eth: move chainSync.forced state type to atomic instead of bool
@ziogaschr found a(nother) race condition
in the tests, which I have had a hard time dupliating
with
env GOMAXPROCS=1 go test -count 1 -race ./eth

But this should, hopefully, fix the issue.

Date: 2023-02-07 08:47:44-08:00
Signed-off-by: meows <[email protected]>
meowsbits committed Feb 7, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit ec625f62412d8f68039ffe3611b94ac445a6cb52
8 changes: 4 additions & 4 deletions eth/sync.go
Original file line number Diff line number Diff line change
@@ -107,7 +107,7 @@ func (h *handler) syncTransactions(p *eth.Peer) {
type chainSyncer struct {
handler *handler
force *time.Timer
forced bool // true when force timer fired
forced uint32 // true when force timer fired
warned time.Time
peerEventCh chan struct{}
doneCh chan error // non-nil when sync is running
@@ -166,7 +166,7 @@ func (cs *chainSyncer) loop() {
case err := <-cs.doneCh:
cs.doneCh = nil
cs.force.Reset(forceSyncCycle)
cs.forced = false
atomic.StoreUint32(&cs.forced, 0)

// If we've reached the merge transition but no beacon client is available, or
// it has not yet switched us over, keep warning the user that their infra is
@@ -176,7 +176,7 @@ func (cs *chainSyncer) loop() {
cs.warned = time.Now()
}
case <-cs.force.C:
cs.forced = true
atomic.StoreUint32(&cs.forced, 1)

case <-cs.handler.quitSync:
// Disable all insertion on the blockchain. This needs to happen before
@@ -210,7 +210,7 @@ func (cs *chainSyncer) nextSyncOp() *chainSyncOp {
}
// Ensure we're at minimum peer count.
minPeers := defaultMinSyncPeers
if cs.forced {
if atomic.LoadUint32(&cs.forced) == 1 {
minPeers = 1
} else if minPeers > cs.handler.maxPeers {
minPeers = cs.handler.maxPeers
11 changes: 6 additions & 5 deletions eth/sync_test.go
Original file line number Diff line number Diff line change
@@ -219,7 +219,8 @@ func TestArtificialFinalityFeatureEnablingDisabling_AbleDisable(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)

next := b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
@@ -235,7 +236,7 @@ func TestArtificialFinalityFeatureEnablingDisabling_AbleDisable(t *testing.T) {
minArtificialFinalityPeers = oMinAFPeers

// Next sync op will unset AF because manager only has 1 peer.
b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next = b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
@@ -306,7 +307,7 @@ func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next := b.handler.chainSync.nextSyncOp()

// Revert safety condition overrides to default values.
@@ -324,7 +325,7 @@ func TestArtificialFinalityFeatureEnablingDisabling_NoDisable(t *testing.T) {
}

// Next sync op will unset AF because manager only has 1 peer.
b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next = b.handler.chainSync.nextSyncOp()
if next != nil {
t.Fatal("non-nil next sync op")
@@ -393,7 +394,7 @@ func TestArtificialFinalityFeatureEnablingDisabling_StaleHead(t *testing.T) {
t.Fatalf("sync failed: %v", err)
}

b.handler.chainSync.forced = true
atomic.StoreUint32(&b.handler.chainSync.forced, 1)
next := b.handler.chainSync.nextSyncOp()

// Revert safety condition overrides to default values.