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

Possible deadlock by recursive calling of parking_lot::read() in one thread in ethcore client #11176

Closed
BurtonQin opened this issue Oct 16, 2019 · 1 comment · May be fixed by #11766
Closed

Possible deadlock by recursive calling of parking_lot::read() in one thread in ethcore client #11176

BurtonQin opened this issue Oct 16, 2019 · 1 comment · May be fixed by #11766
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it.

Comments

@BurtonQin
Copy link
Contributor

  • Parity Ethereum version: 2.6.4
  • Operating system: Windows / MacOS / Linux
  • Installation: built from source
  • Fully synchronized: yes
  • Network: ethereum
  • Restarted: no

Following the bug #11172 , which fixes a possible deadlock by recursive calling of parking_lot::read() in one thread in ethcore client,
I parsed the source code and found 4 more recursive calling of parking_lot::read().
I am afraid it is not an easy job to fix all those bugs.
Maybe a code refactoring is required to change the parking_lot::RwLock into other types of rwlock that allow recursive read lock in one thread.

The bugs are located in

  1. ethcore/src/client/client.rs:386, check_and_lock_block() calls build_last_hashes() on L411
    ethcore/src/client/client.rs:931
  2. ethcore/src/client/client.rs:1949, logs() calls block_number_ref() on L1969
    ethcore/src/client/client.rs:1248 & 1250
  3. ethcore/src/client/client.rs:2102, last_hashes() calls build_last_hashes() on L2102
    ethcore/src/client/client.rs:931
  4. ethcore/src/client/client.rs:2320, prepare_open_block() calls build_last_hashes() on L2331
    ethcore/src/client/client.rs:931

The followings are the source code links:
ethcore/src/client/client.rs:386, check_and_lock_block() calls build_last_hashes() on L411
ethcore/src/client/client.rs:931
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L386-L411
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L919-L931

ethcore/src/client/client.rs:1949, logs() calls block_number_ref() on L1969
ethcore/src/client/client.rs:1248 & 1250
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L1949-L1969
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L1245-L1252

ethcore/src/client/client.rs:2102, last_hashes() calls build_last_hashes() on L2102
ethcore/src/client/client.rs:931
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L2101-L2103
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L919-L931

ethcore/src/client/client.rs:2320, prepare_open_block() calls build_last_hashes() on L2331
ethcore/src/client/client.rs:931
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L2318-L2331
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/ethcore/src/client/client.rs#L919-L931
@niklasad1
Copy link
Collaborator

niklasad1 commented Oct 17, 2019 •
edited
Loading

I think we could use RwLock::read_recursive in those cases that we know that a read lock must be held in order to reach there, then it should not deadlock but may starve writers if the read lock is never dropped (if that happens that is a bug)

But perhaps it is better to look into #7079 instead
long-term

@BurtonQin BurtonQin mentioned this issue Jun 5, 2020
Open
@adria0 adria0 added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 27, 2020
@adria0 adria0 closed this as completed Jul 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it.
Projects
None yet
Milestone
No milestone
3 participants
@adria0 @BurtonQin @niklasad1