Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix lock order when updating sealing #1364

Merged
merged 3 commits into from
Jun 21, 2016
Merged

Fix lock order when updating sealing #1364

merged 3 commits into from
Jun 21, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Jun 20, 2016

No description provided.

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Jun 20, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 20, 2016
@@ -106,7 +106,8 @@ impl Miner {
#[cfg_attr(feature="dev", allow(cyclomatic_complexity))]
fn prepare_sealing(&self, chain: &MiningBlockChainClient) {
trace!(target: "miner", "prepare_sealing: entering");
let transactions = self.transaction_queue.lock().unwrap().top_transactions();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was actually working as expected - the lock should be dropped cause top_transactions is doing a copy.
I didn't wanted to keep transaction queue locked when transactions are executed.

But if you believe it might be dangerous I'm ok with it.

@arkpar arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 21, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 21, 2016
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 21, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 21, 2016
@gavofyork
Copy link
Contributor

@arkpar / @tomusdrw my minor alteration could do with review.

@tomusdrw
Copy link
Collaborator

I think the braces (block) are not really needed in, but can live with them :)

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 21, 2016
@arkpar arkpar merged commit 613d4c9 into master Jun 21, 2016
@debris debris deleted the miner-lock branch June 21, 2016 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants