Fixed concurrency issue that allowed multiple miners instantiation.#777
Fixed concurrency issue that allowed multiple miners instantiation.#777dmitry-worker wants to merge 2 commits intoinput-output-hk:developfrom
Conversation
dmitry-worker
changed the title
|
Good catch, but I think the simpler solution is to use |
I was actually about to implement one of CompareAndSet-based methods, like ^ Those lines create a new actor in a system as a side effect. Although |
| .getOrElse(Future.successful(MinerNotExist)) | ||
| } | ||
|
|
||
| private[this] val mutex = new Object |
There was a problem hiding this comment.
locks/mutex/semaphores are low level, declarative programming approach to concurrencly. They are hard to maintain and super hard to write correctly.
Would it be possible to use sth like STM here. Maybe Ref from cats effect: https://typelevel.org/cats-effect/concurrency/ref.html
| @@ -45,29 +44,31 @@ class EthashConsensus private ( | |||
| blockchainConfig = blockchainConfig | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Is it possible to write a test that will show the problem/solution?
4 participants
Description
Concurrent
EthashConsensus.startMiningProcess()method invocation can lead to several miner actors.There's no thread mutual exclusion and atomicity between
AtomicReference.get()andAtomicReference.set()calls.Proposed Solution
synchronizedorReentrantReadWriteis recommended to ensure that only one miner is created.AtomicReferenceis redundant, since no CAS-related methods are used.Solution assumes that no thread synchronization is needed when miner already exists.