Skip to content

Fix a deadlock in miner.rs #1968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 3, 2020
Merged

Fix a deadlock in miner.rs #1968

merged 7 commits into from
Jun 3, 2020

Conversation

@majecty
Copy link
Contributor

@majecty majecty commented May 21, 2020

There is a deadlock between immune_users and mem_pool.

A client worker thread locks immune_users first and mem_pool second in
prepare_block function.

An HTTP thread locks mem_pool first in import_own_transactions function
and locks immune_users in add_transactions_to_pool function.

It seems that this deadlock is added when we add immune_users lock. We
need to review the lock of immune_users.

@majecty majecty added the bug Something isn't working label May 21, 2020
@majecty majecty requested review from HoOngEe and sgkim126 May 21, 2020 10:10
@HoOngEe
Copy link
Contributor

HoOngEe commented May 21, 2020

How about just moving the line let immune_users = self.immune_users.read(); to the 565th line ?

sgkim126
sgkim126 reviewed May 21, 2020
View reviewed changes
Copy link
Contributor

@sgkim126 sgkim126 left a comment

Choose a reason for hiding this comment

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

Dropping the lock guard of immune_users will not help to fix the deadlock. Since the thread will stop when requiring to lock mem_pool. How about locking mem_pool at the 531st line. To make code always lock mem_pool first?

@majecty
Copy link
Contributor Author

majecty commented May 26, 2020

@HoOngEe @sgkim126 I agree with Seulgi. Locking mem_pool first will be better.

@majecty majecty removed the request for review from HoOngEe June 3, 2020 05:52
@majecty majecty marked this pull request as draft June 3, 2020 05:52
sgkim126
sgkim126 reviewed Jun 3, 2020
View reviewed changes
majecty added 7 commits June 3, 2020 15:54
@majecty
Remove unused TransactionListener
d11d0ee
@majecty
Introduce NextMandatoryReseal in miner.rs
cb89d9a 
NextMandatoryReseal limits the lifetime of the inner lock.
@majecty
Introduce NextAllowedReseal type
bc818b6 
NextAllowedReseal limits the lifetime of the inner lock.
@majecty
Introduce Params struct
9de2bc9 
Params limits the lifetime of the inner lock.
@majecty
Introduce SealingBlockLastRequest
4f8e0a0 
SealingBlockLastRequest limits the lifetime of the inner lock.
@majecty
Introduce Notifiers
1cc11ef 
Nrtofiers limits the lifetime of the inner lock.
@majecty
Introduce Users struct
a9e9700 
Users limits the lifetime of the inner lock.
@majecty majecty force-pushed the f/hotfix-deadlock branch from bfbfce1 to a9e9700 Compare June 3, 2020 07:00
@majecty majecty changed the title Hotfix a deadlock Fix a deadlock in miner.rs Jun 3, 2020
@majecty majecty requested a review from sgkim126 June 3, 2020 07:01
@majecty majecty marked this pull request as ready for review June 3, 2020 07:02
majecty
majecty commented Jun 3, 2020
View reviewed changes
@majecty majecty merged commit 65a0072 into CodeChain-io:rc-2.2.x Jun 3, 2020
@majecty majecty deleted the f/hotfix-deadlock branch June 3, 2020 07:57
This was referenced Jun 3, 2020
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgkim126 sgkim126 sgkim126 approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@majecty @HoOngEe @sgkim126