Python P2P testing bugfixes: rework locking to eliminate race conditions and fix comptool.py #6094
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, each NodeConnCB had its own lock to synchronize data structures used by the testing thread and the networking thread, and NodeConn provided a separate additional lock for synchronizing access to each send buffer. This commit replaces those locks with a single global lock (mininode_lock) that we use to synchronize access to all data structures shared by the two threads. Updates comptool and maxblocksinflight to use the new synchronization semantics, eliminating previous race conditions within comptool, and re-enables invalidblockrequest.py in travis.
|
Tested ACK. |
laanwj
added a commit
that referenced
this pull request
May 4, 2015
2a22d4b Fix comptool send_message call when MAX_INV_SZ reached (Suhas Daftuar) 574db48 Fix potential race conditions in p2p testing framework (Suhas Daftuar) 5487975 Don't run invalidblockrequest.py in travis until race condition is fixed (Suhas Daftuar) ef32817 Fix mininode disconnections to work with select (Suhas Daftuar)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds off of #6089, with more fixes for the python p2p testing framework (see #5981).
Previously, each
NodeConnCBhad its own lock to synchronize data structures used by the testing thread and the networking thread, andNodeConnprovided a separate additional lock for synchronizing access to each send buffer. However, there was no existing lock to protect other data structures that are not specific to a singleNodeConnCB, such asBlockStoreorTxStore, which are created incomptool.pyand shared by all theNodeConnCBinstances.Moreover
comptool.pyhad a race condition because it wasn't synchronizing access to theBlockStore, nor using the existingNodeConnCBlocks before accessing theblock_request_mapinside thecomptool.pyTestNodeobjects. I believe this type of race condition triggered a test failure reported in #5981.This pull tries to simplify the synchronization and make the code more robust to race conditions by replacing all the individual locks one single global lock that we now use to synchronize the two threads. The networking thread acquires this lock before delivering every message;
NodeConn.send_message()uses this lock to synchronize access to thesendbuf; and the thread running the test logic will now acquire this lock before accessing any data shared with one or moreNodeConnCBorNodeConnobjects.