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.

Sign up for GitHub

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

Jump to bottom

miner: fix data race #23435

Merged
merged 4 commits into from Oct 8, 2021
Merged

miner: fix data race #23435

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23d87da
miner: fix data race
MariusVanDerWijden Aug 23, 2021
18d1fef
eth: fix race in testCheckpointChallenge
MariusVanDerWijden Aug 23, 2021
584974d
eth/gasprice: fix race in feehistory due to cache config
MariusVanDerWijden Aug 23, 2021
f65a247
miner: move prefetch stop to mainloop
MariusVanDerWijden Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion eth/gasprice/gasprice_test.go
View file
Open in desktop
Expand Up @@ -146,7 +146,7 @@ func newTestBackend(t *testing.T, londonBlock *big.Int, pending bool) *testBacke
// Construct testing chain
diskdb := rawdb.NewMemoryDatabase()
gspec.Commit(diskdb)
chain, err := core.NewBlockChain(diskdb, nil, gspec.Config, engine, vm.Config{}, nil, nil)
chain, err := core.NewBlockChain(diskdb, &core.CacheConfig{TrieCleanNoPrefetch: true}, gspec.Config, engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("Failed to create local chain, %v", err)
}
Expand Down
1 change: 0 additions & 1 deletion eth/handler_eth_test.go
View file
Open in desktop
Expand Up @@ -490,7 +490,6 @@ func TestCheckpointChallenge(t *testing.T) {
}

func testCheckpointChallenge(t *testing.T, syncmode downloader.SyncMode, checkpoint bool, timeout bool, empty bool, match bool, drop bool) {
t.Parallel()

// Reduce the checkpoint handshake challenge timeout
defer func(old time.Duration) { syncChallengeTimeout = old }(syncChallengeTimeout)
Expand Down
2 changes: 2 additions & 0 deletions miner/worker.go
View file
Open in desktop
Expand Up @@ -318,6 +318,8 @@ func (w *worker) isRunning() bool {
// close terminates all background threads maintained by the worker.
// Note the worker does not support being closed multiple times.
func (w *worker) close() {
w.mu.Lock()
defer w.mu.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I think one better fix is to move the w.current.state.StopPrefetcher() operation into the mainLoop. Since w.current is only available in this loop and you can tear down it in the defer function if the worker is stopped.

Copy link
Contributor

@holiman holiman Sep 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this a bit more...
The worker.close is only called when exitCh is closed, which happens through miner.Close (well, aside from various tests that also use it).

I renamed it like this:

func (miner *Miner) zClose() {
	close(miner.exitCh)
}

And geth still compiles. So I'm wondering, do we actually ever call miner.Close() ->close(exitCh) -> worker.close() ?

if w.current != nil && w.current.state != nil {
w.current.state.StopPrefetcher()
}
Expand Down