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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/rpc/server.h
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 added inline comments.
src/test/test_bitcoin.cpp
87 ↗(On Diff #6657)

Good idea. Done.

jasonbcox marked an inline comment as done.

Rebase + feedback

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.

Fix test for old versions of boost

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

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 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 added inline comments.
src/test/test_bitcoin.cpp
88 ↗(On Diff #7177)

Moved to D2507

jasonbcox marked an inline comment as done.

Rebase + use proper read-write locking

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
  • 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 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 marked 2 inline comments as done.

Better tests and other feedback

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.

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