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

ethcore/test_helpers: fix deadlock caused by double read lock #11767

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BurtonQin
Copy link
Contributor

Similar to #11766,

  1. In block_hash(), the two numbers.read() may be interleaved by numbers.write() from another thread.
    https://github.com/openethereum/openethereum/blob/f8cbdadfaaef30febbeab1ec5b7331a74ec7856a/ethcore/src/test_helpers/test_client.rs#L360
    The fix is to use one numbers.read() instead of two.
  2. In chain_info(), the two difficulty.read()(first_block.read(), ancient_block.read()) may be interleaved by their corresponding write lock from another thread.
    https://github.com/openethereum/openethereum/blob/f8cbdadfaaef30febbeab1ec5b7331a74ec7856a/ethcore/src/test_helpers/test_client.rs#L491-L502
    I lift the read locks and order them as they are declared. Currently, no conflicting lock order is found.

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Nice! I remember we deliberately tried to keep lock as short as possible, but turned out in some situations they must be obtained in advance.

@sorpaas sorpaas added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Jul 29, 2020
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.

2 participants