miner: fix data race #23435
miner: fix data race #23435
Conversation
MariusVanDerWijden
requested review from
holiman,
karalabe and
rjl493456442
as code owners
|
The second commit fixes a datarace between different go routines that all want to update the |
|
The third commit fixes a race in |
|
LGTM so far, but maybe let's wait a few days with merging |
|
The miner fix looks good to me. |
miner/worker.go
Outdated
| @@ -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() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() ?
|
The last change slightly changes the sematics of
I don't whether this change is important or not, but wanted to highlight it so we can figure out whether it matters or not. |
|
The semantics-change is probably fine, if this only ever happens during shutdown. |
|
Actually, there's one more semantic change. This change will cause the prefetcher to be stopped on worker errors: Is that the correct thing to do? Previously it would only happen in conjunction with |
This fixes a data race on worker.current by moving the call to StopPrefetcher into the main loop. The commit also contains fixes for two other races in unit tests of unrelated packages.
This fixes a data race on worker.current by moving the call to StopPrefetcher into the main loop. The commit also contains fixes for two other races in unit tests of unrelated packages.
Fixes a race between
closeandmakeCurrent