Skip to content

Conversation

@pslaski
Copy link
Contributor

@pslaski pslaski commented Dec 30, 2020

Description

Currently, askFor method is designed for sending a message to the actor and waiting for the response. Also, Task was used.
SendMiner prepared to communicate with Miner is using askFor, but not every usage should wait for the response and not every usage was executing task.

Important Changes Introduced

  • divided sendMiner into sendMiner (used only for fire and forget message to the miner) and askMiner (previous sendMiner)

Testing

run and check if miner is starting


override def askMiner(msg: MinerProtocol): Task[MinerResponse] = {
atomicMiner
.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would fix the situation with get() and then...

case Ethash | RestrictedEthash => EthashMiner(node)
case MockedPow => MockedMiner(node)
}
atomicMiner.set(Some(miner))
Copy link
Contributor

Choose a reason for hiding this comment

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

... and then set a miner. Because it is possible to set a miner in between of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitry-worker could you elaborate more?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already PR opened that intend to fix that: #777
There is no conclusion of the solution proposed there is the right one.

The change proposed here is an important fix. IMHO it is better to merge it and deal with potential concurrency issue in PR ☝️ .

@pslaski pslaski merged commit ac0c389 into develop Dec 30, 2020
@pslaski pslaski deleted the fix-not-starting-miner branch December 30, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants