Page MenuHomePhabricator

Added RPCCommand base classes in preparation for RPC migration
Needs RevisionPublic

Authored by jasonbcox on Oct 8 2018, 23:17.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

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.

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpccmd3
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4517
Build 7097: Bitcoin ABC Teamcity Staging
Build 7096: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schancel added inline comments.Nov 28 2018, 20:58
src/rpc/server.cpp
51 ↗(On Diff #6150)

Are we guaranteeing we will always produce a factory for every command even if it's an error factory?

67 ↗(On Diff #6150)

You're going to call a null above with this. You'll want to either change this to return the RPCCommand* and accept the JSONRPCRequest itself, or have a CommandMissingError factory.

src/rpc/server.h
81 ↗(On Diff #6150)

Should this just be RPCCommand *(const JSONRPCRequest &)? Otherwise the comment should say it's returning a factory

This revision now requires changes to proceed.Nov 28 2018, 20:58
jasonbcox marked an inline comment as done.Nov 28 2018, 22:30
jasonbcox added inline comments.
src/rpc/command.h
24 ↗(On Diff #6150)

Add a comment regarding CreateFromJSONRequest and dependency injection. Something like:

rpcServer.RegisterCommand(ArgsTestRPCCommand::commandName, std::bind(&ArgsTestRPCCommand::CreateFromJSONRequest, <dependencies...>));
jasonbcox marked 8 inline comments as done.Nov 29 2018, 21:36
jasonbcox added inline comments.
src/rpc/server.cpp
51 ↗(On Diff #6150)

Good point. Answer should be yes.

67 ↗(On Diff #6150)

Good catch. Fixed.

src/rpc/server.h
81 ↗(On Diff #6150)

Fixed the comment.

jasonbcox updated this revision to Diff 6179.Nov 29 2018, 21:38
jasonbcox marked 3 inline comments as done.

Fixed according to feedback. Always return a factory for MatchCommand, even in the error case.

schancel requested changes to this revision.Nov 29 2018, 22:43
schancel added inline comments.
src/rpc/server.cpp
53 ↗(On Diff #6179)

You will need the JSON rpc error handling catch block here. See the next comment about this.

Possibly wrap the tableRPC.execute command, and move the try {} catch{} from it to up here.

60 ↗(On Diff #6179)

It may be useful to refactor this into a LegacyRPCCommand and have the lookup logic in for tableRPC and the execution.

This revision now requires changes to proceed.Nov 29 2018, 22:43
jasonbcox requested review of this revision.Nov 29 2018, 23:03
jasonbcox marked 4 inline comments as done.
jasonbcox added inline comments.
src/rpc/server.cpp
53 ↗(On Diff #6179)

It appears that this is only necessary because of the transformNamedParameter() logic. At the moment, I'm leaving that out, so adding a try-catch block here is unnecessarily defensive.

60 ↗(On Diff #6179)

Good idea. I'll do this in a future diff since this implementation is strictly simpler.

Fabien added a subscriber: Fabien.Dec 6 2018, 17:59

One small change, looks good otherwise

src/test/rpc_server_tests.cpp
14 ↗(On Diff #6179)

This doesn't seem to be necessary

Fabien requested changes to this revision.Dec 6 2018, 17:59
This revision now requires changes to proceed.Dec 6 2018, 17:59
jasonbcox marked 3 inline comments as done.Dec 21 2018, 22:18
jasonbcox added inline comments.
src/test/rpc_server_tests.cpp
14 ↗(On Diff #6179)

Good catch. This was from an earlier version of this diff.

jasonbcox updated this revision to Diff 6404.Dec 21 2018, 22:19
jasonbcox marked an inline comment as done.

Removed unused include + rebase

deadalnix requested changes to this revision.Dec 23 2018, 15:40
deadalnix added inline comments.
src/rpc/command.h
33 ↗(On Diff #6404)

Why would I register that ugly thing when I can register the command ?

src/rpc/server.h
73 ↗(On Diff #6404)

The function name was kind of an hint that it should take a command as an argument.

What does this do ? It register something. What is that something ? It is a command.

This revision now requires changes to proceed.Dec 23 2018, 15:40
jasonbcox added inline comments.Fri, Dec 28, 22:25
src/rpc/command.h
33 ↗(On Diff #6404)

It's necessary for dependency injection.

src/rpc/server.h
73 ↗(On Diff #6404)

It's technically registering a command's factory function. I can rename this function to RegisterCommandFactory to be more clear, as registering commands requires something for dependency injection. This factory is that something.

jasonbcox updated this revision to Diff 6476.Tue, Jan 1, 01:26

Rebase and rename RegisterCommand() -> RegisterCommandFactory()

jasonbcox planned changes to this revision.Thu, Jan 3, 00:15

Talked with Amaury offline and he clarified how the dependency injection can be simplified. Planning changes until the new changes are done.

jasonbcox updated this revision to Diff 6493.Thu, Jan 3, 20:08

Simplified dependency injection

jasonbcox edited the summary of this revision. (Show Details)Thu, Jan 3, 20:10
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox removed a reviewer: schancel.
jasonbcox added inline comments.Thu, Jan 3, 20:14
src/rpc/command.h
18 ↗(On Diff #5316)

Since we're no longer using the factory pattern, RPCCommand doesn't explicitly have access to JSONRPCRequest anymore. This can be remedied when we migrate the wallet RPC commands using one of these options (currently out of scope of this diff):
A. Execute(UniValue) -> Execute(JSONRPCRequest, UniValue) for all commands
B. Execute(UniValue) -> Execute(JSONRPCRequest) for all commands, but each command must pull the args out of the request via request.params
C. Special case in RPCServer that calls a new Execute(...) in the style of A or B, but only for wallet RPC commands.

deadalnix requested changes to this revision.Thu, Jan 3, 20:58
deadalnix added inline comments.
src/rpc/command.h
21 ↗(On Diff #6493)

I'm not sure what that comment mean or how it's helping me in any way building RPC commands.

29 ↗(On Diff #6493)

I'm fairly confident hooking custom logic on GetName will not be required.

src/rpc/server.cpp
53 ↗(On Diff #6493)

This begs the question of thread safety.

src/rpc/server.h
56 ↗(On Diff #6493)

Who owns the command objects, if not this ?

73 ↗(On Diff #6493)

The fact you need to do this is probably a good sign that the name actually doesn't belong in the command. What if GetName returns a different result every time ? And if it cannot, then it means that GetName's abstraction is actually borken. It seems to depend more on RPCServer's behavior than the command itself.

81 ↗(On Diff #6493)

Does that need to be part of the public API ?

src/test/rpc_server_tests.cpp
16 ↗(On Diff #6493)

class

29 ↗(On Diff #6493)

You are clearly leaking here

45 ↗(On Diff #6493)

You have this oddly recurring pattern in all your commands. The code is trying to tell you something, you just have to listen to it. If you do not, you'll end up iterating forever.

This revision now requires changes to proceed.Thu, Jan 3, 20:58
jasonbcox added inline comments.Thu, Jan 3, 22:05
src/rpc/command.h
29 ↗(On Diff #6493)

The intent was to have each class implement GetName() like such:

std::string GetName() const {
   return "my-command-name";
}

This is a very common pattern used in Java applications.
The alternative is to put the commandName in RPCCommand's constructor.

src/test/rpc_server_tests.cpp
45 ↗(On Diff #6493)

I only did this for testing purposes. I realized it looked odd, but I wanted the string value to be accessible to the test. Alternatively, I can do: request.strMethod = testCommand->GetName(); I just felt it did not test ExecuteCommand as well.

deadalnix added inline comments.Thu, Jan 3, 22:23
src/rpc/command.h
29 ↗(On Diff #6493)

You are defining a function. Functions DO stuff. You make that function vitrual => you want subclasses to extend the behavior, ie DO different stuff.

It's no surprise that this is done with a virtual function in Java, because this is literally the only tool the language provides. Classes with virtual functions. But in that case, even in java, you ave all the signs that the name is likely not in the right place.

jasonbcox updated this revision to Diff 6657.Mon, Jan 14, 23:12
jasonbcox marked 3 inline comments as done.
  • Rebased
  • Fixed pointer ownership of RPCServer::commands
  • RPCCommand::GetName() is no longer unexpectedly virtual
  • Cleaned up the tests and comments
src/rpc/command.h
21 ↗(On Diff #6493)

Updated the wording. Hopefully it makes more sense now.

29 ↗(On Diff #6493)

Makes sense. Fixed.

src/rpc/server.h
56 ↗(On Diff #6493)

this does own the command objects. I was using an old C++ coding style which is pre-unique_ptr. Will update to use those instead.

73 ↗(On Diff #6493)

The commands need to reference their own name to generate help messages, etc. The name should never change.

81 ↗(On Diff #6493)

Yes, for tests.

src/test/rpc_server_tests.cpp
16 ↗(On Diff #6493)

I used struct since everything is public in this definition, but I'm fine with either.

Fabien requested changes to this revision.Thu, Jan 17, 14:05
Fabien added inline comments.
src/rpc/server.h
69

The comment seems outdated

src/test/rpc_server_tests.cpp
21

override

34

Please use BOOST_CHECK_EQUAL wherever you can, it makes debugging any error a lot easier

42

override

src/test/test_bitcoin.cpp
87

Could you please add your phabricator comment here as a code comment ?
The reason for this code is not obvious, having a comment would avoid removing it inappropriately.

This revision now requires changes to proceed.Thu, Jan 17, 14:05