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

Fix a deadlock #9952

Merged
merged 3 commits into from
Nov 25, 2018
Merged

Fix a deadlock #9952

merged 3 commits into from
Nov 25, 2018

Conversation

ngotchac
Copy link
Contributor

A deadlock has been introduced in #8643:
In fn commit(&self) there were:
https://github.com/paritytech/parity-ethereum/blob/9475a2e474d9175abf05b30167bc543bd25a4e3c/ethcore/src/blockchain/blockchain.rs#L1190-L1191

And in fn chain_info(&self) there were:
https://github.com/paritytech/parity-ethereum/blob/9475a2e474d9175abf05b30167bc543bd25a4e3c/ethcore/src/blockchain/blockchain.rs#L1533-L1534

So basically, if chain_info was called while commit was undergoing, it could happen that commit would W-lock self.best_ancient_block, chain_info would R-lock self.best_block, and then commit would wait to lock self.best_block while chain_info would wait to lock self.best_ancient_block.

I'm not sure if there is a better way to fix this, since it's not the first time we had an issue with this (#9385 fixed a deadlock, but broke the informant, then fixed in #9932).

I try to update chain_info so that it could return an Option with try_read, and the informant would just keep trying to get a Report until one is available, but that breaks a dozen of other files. If anyone has a better idea...

This PR also updates the informant, so that it prints info every 5 seconds, instead of previously varying randomly between 5s and 10s.

  - decimal in Mgas/s
  - print every 5s (not randomly between 5s and 10s)
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 22, 2018
Copy link
Contributor

@lexfrl lexfrl left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but the PR looks correct. Hope there'll be a one who'll able to proof it formally..

ethcore/src/blockchain/blockchain.rs Outdated Show resolved Hide resolved
parity/informant.rs Show resolved Hide resolved
@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2018
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 23, 2018
@sorpaas sorpaas merged commit f20f4c7 into master Nov 25, 2018
@sorpaas sorpaas deleted the ng-fix-deadlock branch November 25, 2018 07:53
@5chdn 5chdn added this to the 2.3 milestone Nov 25, 2018
@5chdn 5chdn mentioned this pull request Nov 27, 2018
12 tasks
5chdn pushed a commit that referenced this pull request Nov 27, 2018
* Update informant:
  - decimal in Mgas/s
  - print every 5s (not randomly between 5s and 10s)

* Fix dead-lock in `blockchain.rs`

* Update locks ordering
5chdn added a commit that referenced this pull request Nov 29, 2018
* version: bump beta to 2.2.2

* Add experimental RPCs flag (#9928)

* WiP

* Enable experimental RPCs.

* Keep existing blocks when restoring a Snapshot (#8643)

* Rename db_restore => client

* First step: make it compile!

* Second step: working implementation!

* Refactoring

* Fix tests

* PR Grumbles

* PR Grumbles WIP

* Migrate ancient blocks interating backward

* Early return in block migration if snapshot is aborted

* Remove RwLock getter (PR Grumble I)

* Remove dependency on `Client`: only used Traits

* Add test for recovering aborted snapshot recovery

* Add test for migrating old blocks

* Fix build

* PR Grumble I

* PR Grumble II

* PR Grumble III

* PR Grumble IV

* PR Grumble V

* PR Grumble VI

* Fix one test

* Fix test

* PR Grumble

* PR Grumbles

* PR Grumbles II

* Fix tests

* Release RwLock earlier

* Revert Cargo.lock

* Update _update ancient block_ logic: set local in `commit`

* Update typo in ethcore/src/snapshot/service.rs

Co-Authored-By: ngotchac <[email protected]>

* Adjust requests costs for light client (#9925)

* PIP Table Cost relative to average peers instead of max peers

* Add tracing in PIP new_cost_table

* Update stat peer_count

* Use number of leeching peers for Light serve costs

* Fix test::light_params_load_share_depends_on_max_peers (wrong type)

* Remove (now) useless test

* Remove `load_share` from LightParams.Config
Prevent div. by 0

* Add LEECHER_COUNT_FACTOR

* PR Grumble: u64 to u32 for f64 casting

* Prevent u32 overflow for avg_peer_count

* Add tests for LightSync::Statistics

* Fix empty steps (#9939)

* Don't send empty step twice or empty step then block.

* Perform basic validation of locally sealed blocks.

* Don't include empty step twice.

* prevent silent errors in daemon mode, closes #9367 (#9946)

* Fix a deadlock (#9952)

* Update informant:
  - decimal in Mgas/s
  - print every 5s (not randomly between 5s and 10s)

* Fix dead-lock in `blockchain.rs`

* Update locks ordering

* Fix light client informant while syncing (#9932)

* Add `is_idle` to LightSync to check importing status

* Use SyncStateWrapper to make sure is_idle gets updates

* Update is_major_import to use verified queue size as well

* Add comment for `is_idle`

* Add Debug to `SyncStateWrapper`

* `fn get` -> `fn into_inner`

*  ci: rearrange pipeline by logic (#9970)

* ci: rearrange pipeline by logic

* ci: rename docs script

* fix docker build (#9971)

* Deny unknown fields for chainspec (#9972)

* Add deny_unknown_fields to chainspec

* Add tests and fix existing one

* Remove serde_ignored dependency for chainspec

* Fix rpc test eth chain spec

* Fix starting_nonce_test spec

* Improve block and transaction propagation (#9954)

* Refactor sync to add priority tasks.

* Send priority tasks notifications.

* Propagate blocks, optimize transactions.

* Implement transaction propagation. Use sync_channel.

* Tone down info.

* Prevent deadlock by not waiting forever for sync lock.

* Fix lock order.

* Don't use sync_channel to prevent deadlocks.

* Fix tests.

* Fix unstable peers and slowness in sync (#9967)

* Don't sync all peers after each response

* Update formating

* Fix tests: add `continue_sync` to `Sync_step`

* Update ethcore/sync/src/chain/mod.rs

Co-Authored-By: ngotchac <[email protected]>

* fix rpc middlewares

* fix Cargo.lock

* json: resolve merge in spec

* rpc: fix starting_nonce_test

* ci: allow nightl job to fail
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants