problem: concurrent rw on state db stateobjects map #614
Conversation
solution: use mutex lock fixes #284, again
core/state/statedb.go
Outdated
| if obj := self.stateObjects[addr]; obj != nil { | ||
| self.lock.Unlock() | ||
| if obj.deleted { |
There was a problem hiding this comment.
Сan it be changed here, I mean after Unlock and before return?
There was a problem hiding this comment.
@r8d8 Sorry.. what do you mean?
There was a problem hiding this comment.
(One could use a defer self.lock.Unlock() to unlock before any return point, but that would make the map possibly be locked for the rest of the logic after just reading the value) --
There was a problem hiding this comment.
@r8d8 Maybe I see what you mean. Just pushed a small refactor, I think is better.
whilei
force-pushed
the
fix-statedb-stateobject-concurrent-rw-panic
branch
from
eac354e
to
476a39f
Compare
|
@tzdybal If you have a free second, curious your thoughts on this too. As documented in the original PR, I'm still wondering how and why there aren't MORE locks in this file and package. Is it developer laziness? Is it ok and there's just some concept I'm missing? Since the original change appears to have fixed the reported issue, I'm OK merging this as-is, but just putting the question/seed-of-doubt on the table for consideration. |
|
@whilei I don't understand it. It looks like patching ad hoc, just where it actually crashes. Lock in |
|
Yea.. sketchily ad hoc was my impression too. It seemed to me like the code was somehow usually/luckily "getting away" with not having more locks around the map r/w's. I just threw the patch in there where it crashed because I assumed that I didn't understand why my lock was the first around so many map interactions. |
|
I'm going to go ahead and merge as-is, then we can maybe come back later and add a more consistent locking strategy. Or at least be aware that we're living dangerously. |
solution: use mutex lock
fixes #284, again