This is the scaffolding for soon-to-be RPC refactors. RPCCommand enables easy passing of dependencies only to the RPC commands that use them.
New RPCCommands can be written with appropriate dependency injection and old commands can be slowly migrated to use non-global dependencies.
Details
Details
- Reviewers
deadalnix Fabien - Group Reviewers
Restricted Project - Commits
- rSTAGING97c2227fc9ba: Added RPCCommand base classes in preparation for RPC migration
rABC97c2227fc9ba: Added RPCCommand base classes in preparation for RPC migration
ninja check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- rpccmd2
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4176 Build 6418: Bitcoin ABC Buildbot (legacy) Build 6417: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| src/rpc/server.h | ||
|---|---|---|
| 69 ↗ | (On Diff #6657) | The comment seems outdated |
| src/test/rpc_server_tests.cpp | ||
| 21 ↗ | (On Diff #6657) | override |
| 34 ↗ | (On Diff #6657) | Please use BOOST_CHECK_EQUAL wherever you can, it makes debugging any error a lot easier |
| 42 ↗ | (On Diff #6657) | override |
| src/test/test_bitcoin.cpp | ||
| 87 ↗ | (On Diff #6657) | Could you please add your phabricator comment here as a code comment ? |
| src/test/test_bitcoin.cpp | ||
|---|---|---|
| 87 ↗ | (On Diff #6657) | Good idea. Done. |
| src/rpc/server.cpp | ||
|---|---|---|
| 53 ↗ | (On Diff #6493) | The current RPC system uses signals to trigger various RPC behavior. There is no threading support for executing RPC calls as far as I can find. |
Comment Actions
Still need to design this system to be thread-safe rather than making assumptions about it's use based on current behavior.
| src/rpc/server.cpp | ||
|---|---|---|
| 45 ↗ | (On Diff #7177) | Clearly, it assumed this function may be called from multiple threads or even before warmup has finished, and that this function would be thread safe in such instance. |
| 52 ↗ | (On Diff #7177) | So certainly there is some mechanism making sure this stay alive whatever other threads do while it is being used. |
| 57 ↗ | (On Diff #7177) | Does tableRPC does this ? If not, what is the reason behind this behavior change ? |
| src/test/test_bitcoin.cpp | ||
| 88 ↗ | (On Diff #7177) | Either the bug still exists, and this clearly doesn't fix it, or this actually fixes the bug, in which case the bug doesn't exists anymore in the codebase this comment will be a part of. Either way that doesn't make sense. If this comes due to specificity in the test environment, then explaining what they are instead of complaining about poor test coverage would be more useful. Improving test coverage as well. It seems like this doesn't have much to do with the RPCServer class or the rest of this diff anyways, so it should be split. |
| src/rpc/command.h | ||
|---|---|---|
| 30 ↗ | (On Diff #7208) | Had to add a virtual destructor since shared_ptr used in RWCollection requires it. |
Comment Actions
- Fixed r/w synchronization
- Removed MatchCommand() since it made the implemenation of the above easier (and it was suggested earlier that it was questionable to have in the public API)
| src/rpc/command.h | ||
|---|---|---|
| 15 ↗ | (On Diff #7222) | Most likely, this is non copyable. |
| src/rpc/server.cpp | ||
| 62 ↗ | (On Diff #7222) | I still don't get why there is a try catch here. |
| src/test/rpc_server_tests.cpp | ||
| 13 ↗ | (On Diff #7222) | This is included last |
| 42 ↗ | (On Diff #7222) | You probably want to return something specific and check that it is returned. That would at least prove that the code gets executed. Right now the only thing that's tested is that it doesn't throw, which is not putting the bar very high. |
| 48 ↗ | (On Diff #7222) | Always check that the error yo get is the one you expect. For all you know the test can be fubared and you are testing nothing. |
| src/rpc/command.h | ||
|---|---|---|
| 36 ↗ | (On Diff #7254) | You probably want to return a const std::string& instead of copying the name all the time. |