Hold cs_main in SendMessages() for shorter period of time. #4795

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:2014_send_locking changing 1 files +31 −23

jgarzik commented at 6:10 PM on August 30, 2014: contributor

Much runtime under cs_main seemed unneeded here.

cs_main is a highly contended lock that is held for a long time.

Let's not merge without careful review & testing...
Hold cs_main in SendMessages() for shorter period of time.
Much runtime under cs_main seemed unneeded here.
0731a8dbf4
jgarzik commented at 6:15 PM on August 30, 2014: contributor

There are a couple static variables, but that does not require cs_main.
BitcoinPullTester commented at 6:20 PM on August 30, 2014: none
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4795_0731a8dbf41b781bb1287c453ec4d711a7dcdd54/ for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

in src/main.cpp:None in 0731a8dbf4

4248 | @@ -4249,13 +4249,39 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
4249 |              }
4250 |          }
4251 |  
4252 | -        TRY_LOCK(cs_main, lockMain); // Acquire cs_main for IsInitialBlockDownload() and CNodeState()
4253 | -        if (!lockMain)
4254 | -            return true;
4255 | +        bool isIBD;
4256 | +        CNodeState state;

sipa commented at 8:07 PM on August 30, 2014:

Making a copy of state sounds dangerous, as there is global state that contains iterators to entries in it.

sipa commented at 8:09 PM on August 30, 2014: member

Decreasing the time cs_main is locked (or even splitting it up further), but the largest offender by far is ProcessBlock. Trying to run SendMessages without cs_main sounds like asking for trouble, though.
sipa commented at 8:18 PM on August 30, 2014: member

Also, please let's get #4230 reviewed/merged first.
jgarzik closed this on Sep 4, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

jgarzik

BitcoinPullTester

sipa