RPC: lock push-down, preparing for parallelization opportunities #1494

pull jgarzik wants to merge 3 commits into bitcoin:master from jgarzik:unlocked-rpc-v2 changing 2 files +97 −7

jgarzik commented at 3:14 AM on June 21, 2012: contributor

As an alternative to req #1493, this set of commits pushes the lock down into each RPC function itself.

From there, locking may be improved on a case-by-case basis.
RPC: prepare to parallelization by pushing down LOCK2() into RPC actors
No behavior changes. Simply pushes lock down into each RPC function.
7dcdb38e49

RPC: locking cleanup, the low-hanging fruit

Now that locks have been pushed down, we may remove locks in some obvious
cases (help, stop), move the lock-taking down below initial param parsing in
several cases, and correct a bug in one case (use cs_vNodes).

34e128d18a

luke-jr commented at 3:17 AM on June 21, 2012: member

I like this one a lot better.
RPC: don't hold unnecessary locks in getwork, getmemorypool, getblock[hash] 957f0899fc
gavinandresen commented at 3:49 PM on June 22, 2012: contributor

This is the way we used to do it (each RPC command acquired the locks it needed), and we were constantly battling people adding new RPC commands that worked in testing and then blew up in production, because they didn't acquire the right locks, or they acquired the right locks in the wrong order.

I like the other approach: make the default safe, but if you know what you're doing you can do your own locking.

in src/bitcoinrpc.cpp:None in 957f0899fc

 543 | @@ -531,6 +544,8 @@ Value getinfo(const Array& params, bool fHelp)
 544 |              "getinfo\n"
 545 |              "Returns an object containing various state info.");
 546 |  
 547 | +    LOCK2(cs_main, pwalletMain->cs_wallet);

TheBlueMatt commented at 3:14 PM on June 27, 2012:

Also need cs_vNodes a few lines down.

laanwj commented at 11:34 AM on July 18, 2012: member

The other pull is safer. It allows for a slow transition to finer-grained locking, optimizing where it matters. This is a big change at once, and it's easy to forget one or two locks.
BitcoinPullTester commented at 7:13 PM on August 9, 2012: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/957f0899fc5ecefd2f63fdb0a65527b3252e2bed for binaries and test log.

This pull does not merge cleanly onto current master
jgarzik commented at 3:50 PM on August 21, 2012: contributor

Closing. We're going with the table driven approach for now (#1493).
jgarzik closed this on Aug 21, 2012
jgarzik deleted the branch on Aug 24, 2014
suprnurd referenced this in commit 1d67d52122 on Dec 5, 2017
lateminer referenced this in commit 3ea8c577d4 on May 6, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1493 RPC: add facility to enable RPCs to run outside cs_main, wallet locks