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

Fix deadlock in blockchain. #8977

Merged
merged 2 commits into from Jun 26, 2018
Merged

Fix deadlock in blockchain. #8977

merged 2 commits into from Jun 26, 2018

Conversation

tomusdrw
Copy link
Collaborator

From https://docs.rs/parking_lot/0.6.2/parking_lot/type.RwLock.html

This lock uses a task-fair locking policy which avoids both reader and writer starvation. This means that readers trying to acquire the lock will block even if the lock is unlocked when there are writers waiting to acquire the lock. Because of this, attempts to recursively acquire a read lock within a single thread may result in a deadlock.

And it seems that first_block_number() actually goes through block_number(_) where we check the best_block again

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 25, 2018
andresilva
andresilva approved these changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM

@5chdn
Copy link
Contributor

5chdn commented Jun 26, 2018

Please indicate whether we need to backport this.

@5chdn 5chdn added this to the 1.12 milestone Jun 26, 2018
@tomusdrw
Copy link
Collaborator Author

@5chdn we do, label added.

@debris
Copy link
Collaborator

debris commented Jun 26, 2018

When was this bug introduced? It seems like parity should deadlock every time this function is called

@debris debris added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 26, 2018
@tomusdrw
Copy link
Collaborator Author

@debris it deadlocks randomly and only when it happens that there is a write() lock request in between the two reads. I think the change was introduced in my header refactoring PR I remember touching some stuff with the best_block code.

@5chdn
Copy link
Contributor

5chdn commented Jun 26, 2018

Please rebase on latest master to fix CI.

@5chdn 5chdn mentioned this pull request Jun 26, 2018
Merged
3 tasks
@5chdn 5chdn merged commit 19a6725 into master Jun 26, 2018
@5chdn 5chdn deleted the td-deadlock branch June 26, 2018 18:49
@5chdn 5chdn mentioned this pull request Jun 26, 2018
Merged
10 tasks
5chdn pushed a commit that referenced this pull request Jun 26, 2018
@tomusdrw @5chdn
Fix deadlock in blockchain. (#8977)
d921e2c
5chdn pushed a commit that referenced this pull request Jun 26, 2018
@tomusdrw @5chdn
Fix deadlock in blockchain. (#8977)
7484749
ordian added a commit to ordian/parity that referenced this pull request Jun 27, 2018
@ordian
Merge branch 'master' of https://github.com/paritytech/parity into wa…
e51d0ca 
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  Fix deadlock in blockchain. (openethereum#8977)
  snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (openethereum#8984)
  Use local parity-dapps-glue instead of crate published at crates.io (openethereum#8983)
@tomusdrw tomusdrw mentioned this pull request Jun 27, 2018
Closed
dvdplm added a commit that referenced this pull request Jun 27, 2018
@dvdplm
Merge branch 'master' into refactor/hashdb-generic
550f852 
* master:
  Refactor evm Instruction to be a c-like enum (#8914)
  Fix deadlock in blockchain. (#8977)
  snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984)
  Use local parity-dapps-glue instead of crate published at crates.io (#8983)
  parity: omit redundant last imported block number in light sync informant (#8962)
  Disable hardware-wallets on platforms that don't support `libusb` (#8464)
  Bump error-chain and quick_error versions (#8972)
dvdplm added a commit that referenced this pull request Jun 27, 2018
@dvdplm
Merge branch 'master' into chore/fix-tempdir-dependency
e7c528c 
* master:
  Refactor evm Instruction to be a c-like enum (#8914)
  Fix deadlock in blockchain. (#8977)
  snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984)
  Use local parity-dapps-glue instead of crate published at crates.io (#8983)
  parity: omit redundant last imported block number in light sync informant (#8962)
  Disable hardware-wallets on platforms that don't support `libusb` (#8464)
  Bump error-chain and quick_error versions (#8972)
5chdn added a commit that referenced this pull request Jun 28, 2018
@5chdn
Stable: Backports (#8986)
bd20c45 
* snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984)

* snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530

* snap: use plugin rust

* Fix deadlock in blockchain. (#8977)

* Remove js-glue from workspace

This fixes test error on Rust 1.27 but also prevents js-glue from building itself. Builtin dapp users can still use
js-glue from crates.io.
5chdn added a commit that referenced this pull request Jun 29, 2018
@5chdn
Bump beta to 1.11.5 / Backports (#8955)
0487c5b 
* parity-version: bump beta to 1.11.5

* Update ropsten.json (#8926)

* Update hardcoded headers (#8925)

* Update kovan.json

Update Kovan to block 7693549

* Update foundation.json

Updated to block #5812225

* Update ropsten.json

Update to 3465217

* Update ropsten.json

use tabs

* Update foundation.json

use tabs

* Update kovan.json

use tabs

* scripts: minor improvements (#8930)

* CI: enable 'latest' docker tag on master pipeline

* CI: mark both beta and stable as stable snap.

* CI: sign all windows binaries

* scripts: fix docker build tag on latest using master (#8952)

* rpc: cap gas limit of local calls (#8943)

* snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984)

* snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530

* snap: use plugin rust

* Fix deadlock in blockchain. (#8977)

* Remove js-glue from workspace

This fixes test error on Rust 1.27 but also prevents js-glue from building itself. Builtin dapp users can still use
js-glue from crates.io.

* Fix Android build on beta (#9003)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@folsen folsen folsen approved these changes

@andresilva andresilva andresilva approved these changes

@debris debris debris approved these changes

Assignees
No one assigned
Labels
A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.0 Ethereum
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tomusdrw @5chdn @debris @folsen @andresilva