Skip to content

Remove main.h dependency from rpcserver #5107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
CodeShark wants to merge 10 commits into from
Closed

Remove main.h dependency from rpcserver #5107

CodeShark wants to merge 10 commits into from

Conversation

@CodeShark
Copy link
Contributor

@CodeShark CodeShark commented Oct 20, 2014

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.

@sipa
Copy link
Member

sipa commented Oct 20, 2014

One small nit before actual review: you're committing a Makefile to the repository.

@CodeShark CodeShark force-pushed the No_main_in_rpcserver branch from 51d076f to 8eeaa6b Compare October 20, 2014 06:30
@CodeShark
Copy link
Contributor Author

CodeShark commented Oct 20, 2014

Doh - dunno how that got in there...should be fixed now.

@CodeShark CodeShark force-pushed the No_main_in_rpcserver branch from 8eeaa6b to 247b63d Compare October 20, 2014 06:35
@CodeShark
Copy link
Contributor Author

CodeShark commented Oct 20, 2014

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.

laanwj
laanwj reviewed Oct 20, 2014
View reviewed changes
src/init.cpp Outdated
Copy link
Member

@laanwj laanwj Oct 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either remove these debugging statements or replace them with category-specific LogPrint()

Copy link
Contributor Author

@CodeShark CodeShark Oct 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove all LogPrint() once we've tested.

@CodeShark CodeShark force-pushed the No_main_in_rpcserver branch 2 times, most recently from 7bf84ed to e7118f1 Compare October 21, 2014 01:44
Diapolo
Diapolo reviewed Oct 21, 2014
View reviewed changes
src/rpcmisc.cpp Outdated
Copy link

@Diapolo Diapolo Oct 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can you make this a 2-liner? Same nit for the change below.

@CodeShark CodeShark force-pushed the No_main_in_rpcserver branch 2 times, most recently from cbd3622 to 0351028 Compare October 22, 2014 03:47
@CodeShark CodeShark force-pushed the No_main_in_rpcserver branch from 0351028 to c1f7bb6 Compare October 22, 2014 03:57
CodeShark
CodeShark reviewed Oct 22, 2014
View reviewed changes
src/sync.h
Copy link
Contributor Author

@CodeShark CodeShark Oct 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@laanwj
Copy link
Member

laanwj commented Oct 22, 2014

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)

@laanwj
Copy link
Member

laanwj commented Oct 27, 2014

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

laanwj
laanwj reviewed Oct 30, 2014
View reviewed changes
src/init.cpp
Copy link
Member

@laanwj laanwj Oct 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@laanwj
Copy link
Member

laanwj commented Oct 30, 2014

utACK, going to test this

@sipa
Copy link
Member

sipa commented Nov 4, 2014

utACK

This was referenced Jan 25, 2015
Closed
Merged
@laanwj
Copy link
Member

laanwj commented Jan 26, 2015

Closing in favor of rebased #5711

@laanwj laanwj closed this Jan 26, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CodeShark @sipa @laanwj @Diapolo