Page MenuHomePhabricator

Add HTTPRPCRequestProcessor to own httprpc
ClosedPublic

Authored by jasonbcox on Sep 6 2018, 22:37.

Details

Summary

Adding HTTPRPCRequestProcessor provides an easy way for Config and any future (RPCServer) references to be passed around between HTTP and RPC servers.

Test Plan

make check && test_runner.py
Also tested RPC directly:
bitcoind -regtest
bitcoin-cli -regtest getexcessiveblock
It works!

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

jasonbcox created this revision.Sep 6 2018, 22:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 6 2018, 22:37
jasonbcox added inline comments.Sep 6 2018, 22:39
src/httprpc.cpp
338 ↗(On Diff #4808)

It's entirely possible to put this, and many of the other functions in this file, into the HTTPRPCServer class as member functions. I figured that was best left for a future diff. Please let me know if you disagree.

deadalnix requested changes to this revision.Sep 7 2018, 13:16

Looks like the Qt initialization is some special level of fubared, but we'll keep that part for another day.

src/httprpc.cpp
344 ↗(On Diff #4808)

Passing the this pointer down to function is ABI dependent, so my bet would be that this is undefined behavior. The best option here is to create a static function taking a pointer to a HTTPRPCServer and other parameters and forward the call. You can bind the server's reference to that function.

This will also allow you t make ProcessHTTPRequest private which as per my previous comment seems like it should be.

src/httprpc.h
24 ↗(On Diff #4808)

Now that seems like a bizarre API. What's the intended use of that class ?

Should we have some code listening to the port 80, build HTTP request and then send it to the HTTPRPCServer ? I would argue that it is the responsibility of the HTTPRPCServer to listen to the port 80, not to receive request from a 3rd party (in which case it would a HTTPRPCRequestProcessor or something like this).

52 ↗(On Diff #4808)

It seems liek a of nits could go in their own patch, that would make that diff much clearer.

87 ↗(On Diff #4808)

Why was this moved into the header ?

This revision now requires changes to proceed.Sep 7 2018, 13:16
jasonbcox planned changes to this revision.Sep 8 2018, 21:57
jasonbcox added inline comments.
src/httprpc.h
52 ↗(On Diff #4808)

Done: D1742

87 ↗(On Diff #4808)

I moved it preempting using it as a private member in the new class, but didn't end up adding it yet. It doesn't belong in this diff. I'll remove it.

jasonbcox added inline comments.Sep 8 2018, 22:03
src/httprpc.h
24 ↗(On Diff #4808)

I'll rename to HTTPRPCRequestProcessor. This class is not intended to listen like a HTTP server as you pointed out. It's more of a bridge between the HTTP server and RPC server.

jasonbcox added inline comments.Sep 9 2018, 05:22
src/httprpc.cpp
344 ↗(On Diff #4808)

This will require me to add HTTPRPCServer to the API of RegisterHTTPHandler, which is called by the httpserver code as well (not just here). Do you think that is acceptable? The most common use case for RegisterHTTPHandler will not end up using it.

Ideally, we should be able register HTTP handlers with different dependencies, but that will require reworking a bunch of the httpserver code.

Thoughts?

jasonbcox updated this revision to Diff 4980.Sep 20 2018, 22:32

Updated according to most feedback. Work on RegisterHTTPHandler() is still in progress.

jasonbcox marked 6 inline comments as done.Sep 20 2018, 22:38
jasonbcox retitled this revision from Add HTTPRPCServer to own httprpc to Add HTTPRPCRequestProcessor to own httprpc.Sep 25 2018, 22:29
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox updated this revision to Diff 5077.Sep 26 2018, 19:07

Fixed binding so that it doesn't call the delegate directly

deadalnix added inline comments.Sep 26 2018, 19:19
src/httprpc.cpp
344 ↗(On Diff #5077)

You can define this in the header.

380 ↗(On Diff #5077)

Why is std::placeholders::_2 required here ?

jasonbcox marked an inline comment as done.Sep 26 2018, 19:22
jasonbcox added inline comments.
src/httprpc.cpp
380 ↗(On Diff #5077)

It is essentially a definition that specifies which parameter to pass down to the handler function. In this case, it's passing parameter 2, which is the HTTPRequest* (see the std::function<>() definition above for reference of the params that the handler-caller provides)

deadalnix accepted this revision.Sep 26 2018, 19:25
This revision is now accepted and ready to land.Sep 26 2018, 19:25
jasonbcox updated this revision to Diff 5082.Sep 26 2018, 20:43

Moved static function implementation to header

This revision was automatically updated to reflect the committed changes.