Remove main.h dependency from rpcserver #5107
Conversation
|
One small nit before actual review: you're committing a Makefile to the repository. |
51d076f to
8eeaa6b
Compare
|
Doh - dunno how that got in there...should be fixed now. |
8eeaa6b to
247b63d
Compare
|
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. |
|
|
||
| void OnRPCPreCommand(const CRPCCommand& cmd) | ||
| { | ||
| LogPrintf("pre-RPC command: %s\n", cmd.name.c_str()); |
There was a problem hiding this comment.
Please either remove these debugging statements or replace them with category-specific LogPrint()
There was a problem hiding this comment.
I'll remove all LogPrint() once we've tested.
7bf84ed to
e7118f1
Compare
| @@ -70,6 +70,11 @@ Value getinfo(const Array& params, bool fHelp) | |||
| + HelpExampleRpc("getinfo", "") | |||
| ); | |||
|
|
|||
| ENTER_CRITICAL_SECTION(cs_main); | |||
| #ifdef ENABLE_WALLET | |||
| if (pwalletMain) ENTER_CRITICAL_SECTION(pwalletMain->cs_wallet); | |||
There was a problem hiding this comment.
Nit: Can you make this a 2-liner? Same nit for the change below.
cbd3622 to
0351028
Compare
0351028 to
c1f7bb6
Compare
| else | ||
| Enter(pszName, pszFile, nLine); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
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) |
| @@ -215,6 +215,26 @@ bool static Bind(const CService &addr, unsigned int flags) { | |||
| return true; | |||
| } | |||
|
|
|||
| void OnRPCStopped() | |||
There was a problem hiding this comment.
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
|
utACK, going to test this |
|
utACK |
|
Closing in favor of rebased #5711 |
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:
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.
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.
Ultimately, it would be a good idea to have the mining rpc unit register its own handler and contain this mechanism to this unit.