Adding HTTPRPCRequestProcessor provides an easy way for Config and any future (RPCServer) references to be passed around between HTTP and RPC servers.
Details
Details
- Reviewers
deadalnix schancel Mengerian - Group Reviewers
Restricted Project - Commits
- rSTAGING12dfab4446b5: Add HTTPRPCRequestProcessor to own httprpc
rABC12dfab4446b5: Add HTTPRPCRequestProcessor to own httprpc
make check && test_runner.py
Also tested RPC directly:
bitcoind -regtest
bitcoin-cli -regtest getexcessiveblock
It works!
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D1736
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3335 Build 4753: Bitcoin ABC Buildbot (legacy) Build 4752: arc lint + arc unit
Event Timeline
| 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. |
Comment Actions
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 ? |
| 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. |
| 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? |
Comment Actions
Updated according to most feedback. Work on RegisterHTTPHandler() is still in progress.
| 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) |