Page MenuHomePhabricator

Added RPCCommand base classes in preparation for RPC migration
ClosedPublic

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

Details

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.

Depends on D2500, D2507

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox added inline comments.Jan 14 2019, 23:12
src/rpc/command.h
29 ↗(On Diff #6493)

Makes sense. Fixed.

src/rpc/server.h
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.Jan 17 2019, 14:05
Fabien added inline comments.
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 ?
The reason for this code is not obvious, having a comment would avoid removing it inappropriately.

This revision now requires changes to proceed.Jan 17 2019, 14:05
jasonbcox marked 5 inline comments as done.Feb 4 2019, 19:59
jasonbcox added inline comments.
src/test/test_bitcoin.cpp
87 ↗(On Diff #6657)

Good idea. Done.

jasonbcox updated this revision to Diff 7169.Feb 4 2019, 19:59
jasonbcox marked an inline comment as done.

Rebase + feedback

jasonbcox planned changes to this revision.Feb 4 2019, 20:05
jasonbcox requested review of this revision.Feb 4 2019, 22:17
jasonbcox added inline comments.
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.

jasonbcox updated this revision to Diff 7176.Feb 4 2019, 23:29

Fix test for old versions of boost

jasonbcox planned changes to this revision.Feb 4 2019, 23:30

Still need to design this system to be thread-safe rather than making assumptions about it's use based on current behavior.

jasonbcox updated this revision to Diff 7177.Feb 4 2019, 23:57

Added a lock for registering commands in a thread-safe way

deadalnix requested changes to this revision.Feb 5 2019, 15:06
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Feb 5 2019, 15:06
jasonbcox planned changes to this revision.Feb 6 2019, 17:37
jasonbcox added inline comments.
src/rpc/server.cpp
35 ↗(On Diff #7177)

should not be global

44 ↗(On Diff #7177)

Verify and comment regarding warmup and node state

52 ↗(On Diff #7177)

I need to use RWCollection

57 ↗(On Diff #7177)

It does. See CRPCTable::execute(...) below.

jasonbcox marked 4 inline comments as done.Feb 7 2019, 01:00
jasonbcox added inline comments.
src/test/test_bitcoin.cpp
88 ↗(On Diff #7177)

Moved to D2507

jasonbcox updated this revision to Diff 7208.Feb 7 2019, 01:01
jasonbcox marked an inline comment as done.

Rebase + use proper read-write locking

jasonbcox added inline comments.Feb 7 2019, 01:04
src/rpc/command.h
30 ↗(On Diff #7208)

Had to add a virtual destructor since shared_ptr used in RWCollection requires it.

deadalnix requested changes to this revision.Feb 7 2019, 13:04

The synchronization is still incorrect.

This revision now requires changes to proceed.Feb 7 2019, 13:04
jasonbcox updated this revision to Diff 7222.Feb 7 2019, 21:51
  • 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)
deadalnix requested changes to this revision.Feb 8 2019, 18:07
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Feb 8 2019, 18:07
jasonbcox marked 4 inline comments as done.Feb 8 2019, 23:34
jasonbcox added inline comments.
src/rpc/server.cpp
62 ↗(On Diff #7222)

Misunderstanding from earlier.

src/test/rpc_server_tests.cpp
48 ↗(On Diff #7222)

I was having trouble finding something like BOOST_CHECK_EXCEPTION (until I did). My fallback was similar behavior that others tests are using. I've fixed this now.

jasonbcox updated this revision to Diff 7254.Feb 8 2019, 23:34
jasonbcox marked 2 inline comments as done.

Better tests and other feedback

deadalnix accepted this revision.Feb 9 2019, 20:11
deadalnix added inline comments.
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.

Fabien accepted this revision.Feb 11 2019, 20:07
This revision is now accepted and ready to land.Feb 11 2019, 20:07
This revision was automatically updated to reflect the committed changes.