Remove main.h dependency from rpcserver #5107

pull CodeShark wants to merge 10 commits into bitcoin:master from CodeShark:No_main_in_rpcserver changing 11 files +229 −36
  1. CodeShark commented at 5:37 am on October 20, 2014: contributor

    This is a step in the direction of completely decoupling the RPC server from the rest of the application.

    This commit specifically addresses three issues:

    1. Thread locking of RPC commands at the level of server dispatch is not a good idea for two reasons:
    • the RPC server shouldn’t require any application-specific knowledge nor global synchronization objects (i.e. cs_main and cs_wallet)
    • we completely lose the ability to optimize concurrency; it is much better to push these locks further down the call stack, ideally just around the lines of code where contention arises. For now, we take a very conservative approach and just push them down a single level by wrapping the individual non-thread-safe RPC command functions in locks.

    This solution is incomplete - it is only intended to move us in the right direction. The locks are still too coarse, and some locks appear unnecessary altogether (i.e. decoderawtransaction). We just stick to the command table flags as is for now to preserve existing behavior.

    1. Certain RPC commands might be disabled in certain contexts (i.e. safe mode, no wallet loaded). Rather than having the RPC server handle this logic, it is preferable to have the application subscribe handlers that can throw exceptions before attempting to execute the command. So we’ve added a handler that gets called prior to executing the command, where the application has the opportunity to throw an exception. Another handler has been added which gets called after executing the command which is currently unused, but might be useful later on.

    This solution is also incomplete. The RPC server unit still defines the command table structure, which is application-specific knowledge. This is a step in the direction of mapping method names to abstract callable objects that can be subclassed and registered by the application.

    1. The getblocktemplate method uses long-polling which means the connection thread must be woken up when the RPC server is shut down. Rather than signaling the condition variable cvBlockChange directly when the RPC server is stopping, it is better to provide a general signaling mechanism to let the application subscribe handlers when the server starts and stops.

    Ultimately, it would be a good idea to have the mining rpc unit register its own handler and contain this mechanism to this unit.

  2. sipa commented at 5:56 am on October 20, 2014: member
    One small nit before actual review: you’re committing a Makefile to the repository.
  3. CodeShark force-pushed on Oct 20, 2014
  4. CodeShark commented at 6:31 am on October 20, 2014: contributor
    Doh - dunno how that got in there…should be fixed now.
  5. CodeShark force-pushed on Oct 20, 2014
  6. Removed main.h dependency from rpcserver.cpp 52227f7b3d
  7. CodeShark commented at 8:24 am on October 20, 2014: contributor
    Before these commits, the getblocktemplate thread was being woken up before joining the thread. I had moved the signal to the end of StopRPCThreads, but I think we could end up with a deadlock if we wait to signal until after joining the thread.
  8. in src/init.cpp: in f9315d0793 outdated 
    221+    LogPrintf("RPC stopped.\n");
222+}
223+
224+void OnRPCPreCommand(const CRPCCommand& cmd)
225+{
226+    LogPrintf("pre-RPC command: %s\n", cmd.name.c_str());
    laanwj commented at 11:12 am on October 20, 2014:
    Please either remove these debugging statements or replace them with category-specific LogPrint()
    CodeShark commented at 11:18 pm on October 20, 2014:
    I’ll remove all LogPrint() once we’ve tested.
  9. CodeShark force-pushed on Oct 20, 2014
  10. CodeShark force-pushed on Oct 21, 2014
  11. in src/rpcmisc.cpp: in e7118f1765 outdated 
    69@@ -70,6 +70,11 @@ Value getinfo(const Array& params, bool fHelp)
70             + HelpExampleRpc("getinfo", "")
71         );
72 
73+    ENTER_CRITICAL_SECTION(cs_main);
74+#ifdef ENABLE_WALLET
75+    if (pwalletMain) ENTER_CRITICAL_SECTION(pwalletMain->cs_wallet);
    Diapolo commented at 10:44 am on October 21, 2014:
    Nit: Can you make this a 2-liner? Same nit for the change below.
  12. in src/rpcserver.cpp: in e7118f1765 outdated 
     6@@ -7,9 +7,9 @@
 7 
 8 #include "base58.h"
 9 #include "init.h"
10-#include "main.h"
11 #include "ui_interface.h"
12 #include "util.h"
13+#include "random.h"
    Diapolo commented at 10:44 am on October 21, 2014:
    Nit: random should be placed where main was (alphabetical ordering).
  13. in src/rpcserver.cpp: in e7118f1765 outdated 
    23@@ -24,12 +24,14 @@
24 #include <boost/iostreams/stream.hpp>
25 #include <boost/shared_ptr.hpp>
26 #include <boost/thread.hpp>
27+#include <boost/signals2/signal.hpp>
    Diapolo commented at 10:45 am on October 21, 2014:
    Nit: Should be above thread (alphabetical ordering).
  14. in src/rpcserver.cpp: in e7118f1765 outdated 
    29 
30 using namespace boost;
31 using namespace boost::asio;
32 using namespace json_spirit;
33 using namespace std;
34+using namespace RPCServer;
    Diapolo commented at 10:45 am on October 21, 2014:
    Nit: Should be above std (alphabetical ordering).
  15. CodeShark force-pushed on Oct 22, 2014
  16. CodeShark force-pushed on Oct 22, 2014
  17. Enable conditional locking. c828231183
  18. CodeShark force-pushed on Oct 22, 2014
  19. Use conditional lock in signrawtransaction. 522ad03da5
  20. Use conditional lock in getinfo and validateaddress. b3d0bc72db
  21. Allow recursive locking in getmininginfo. a419d09fcb
  22. Added RPC signals. 8a022a8dac
  23. Alphabetical ordering of includes and namespaces c1f7bb600a
  24. in src/sync.h: in c1f7bb600a outdated 
    150+        if (fTry)
151+            TryEnter(pszName, pszFile, nLine);
152+        else
153+            Enter(pszName, pszFile, nLine);
154+    }
155+
    CodeShark commented at 5:07 am on October 22, 2014:

    This should be seen as a minimally-invasive temporary fix, allowing us to use RAII on mutexes whose existence we can only know about at runtime (i.e. pwalletMain->cs_wallet).

    The permanent fix will be to move all functionality depending on pwalletMain->cs_wallet into separate functions altogether - or perhaps to encapsulate locking within the wallet class itself.

  25. laanwj commented at 9:23 am on October 22, 2014: member

    Concept ACK, I now like the approach you take to pushing down the locks.

    (someone will have to verify that all the RPC methods get the right locks, but this is easier to review than functional changes inside the methods)

  26. laanwj added the label Improvement on Oct 22, 2014
  27. laanwj commented at 5:32 pm on October 27, 2014: member

    I checked locking for all RPC calls after your change.

    Non-threadsafe RPC functions

    according to the table in https://github.com/CodeShark/bitcoin/blob/No_main_in_rpcserver/src/rpcserver.cpp :

    Threadsafe RPC functions:

    • help: No lock as expected
    • stop: No lock as expected
    • addnode: No lock as expected
    • getaddednodeinfo: No lock as expected
    • getnettotals: No lock as expected
    • getmempoolinfo: No lock as expected
    • submitblock: No lock as expected
    • setgenerate: No lock as expected
    • createmultisig: No lock as expected
    • estimatefee: No lock as expected
    • estimatepriority: No lock as expected

    Wallet:

    Conclusion:

    • Locks missing in dumpprivkey, dumpwallet, importprivkey, importwallet, importaddress (rpcdump.cpp)
    • Locks missing in listunspent (rpcrawtransaction.cpp)
    • All other functions have the necessary locks
  28. Added locks to dumpprivkey, dumpwallet, importprivkey, importwallet, and importaddress. 1fc6d421f8
  29. Added locks to listunspent. d21569e7a7
  30. Missing include 3394388ef0
  31. in src/init.cpp: in 3394388ef0 
    214@@ -215,6 +215,26 @@ bool static Bind(const CService &addr, unsigned int flags) {
215     return true;
216 }
217 
218+void OnRPCStopped()
    laanwj commented at 4:39 pm on October 30, 2014:
    Not sure where to move them otherwise (probably a bitcoin-specific RPC server module, where the method registration belongs too) but it’s not very nice to have these RPC-specific implementation functions in init.cpp
  32. laanwj commented at 4:48 pm on October 30, 2014: member
    utACK, going to test this
  33. sipa commented at 1:39 pm on November 4, 2014: member
    utACK
  34. laanwj commented at 12:56 pm on January 26, 2015: member
    Closing in favor of rebased #5711
  35. laanwj closed this on Jan 26, 2015
  36. MarcoFalke locked this on Sep 8, 2021


CodeShark sipa laanwj Diapolo

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-19 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me