Two issues with sendThread, both surfaced by an independent review:
UAF: the sending job captured a raw `this`. An admin Unsubscribe runs
NetworkOPsImp::tryRemoveRpcSub, which erases the sub from mRpcSubMap
under mSubLock and destroys it inline — while sendThread may be running
on a job-queue worker under the *unrelated* InfoSub::mLock. The blocking
io_service.run() (up to the 30s request deadline for a hung endpoint)
makes the window large, and a hung endpoint is exactly this fix's
scenario. RPCSubImp now derives enable_shared_from_this and the job
captures weak_from_this(), re-locking it on entry so the sub stays alive
for the batch; once it's gone the job is a no-op.
Starvation: sendThread drained the entire backlog (up to 16384 events,
512 batches x up to 30s) in one job, blocking a worker thread the whole
time. With a small pool shared with consensus/ledger/RPC, a few hung
subscribers could occupy most workers. sendThread now processes ONE
batch per job and re-queues if more remain, so the worker is released
between batches and other jobs interleave.
The RPCSub tests crashed/timed out under the instrumented Debug coverage
build (where the full HTTP stack is ~50x slower than Release):
- Deterministic teardown: wait on JobQueue::getJobCountTotal(
jtCLIENT_SUBSCRIBE) == 0 before destroying the sub. sendThread captures
a raw `this`; a fixed-sleep grace raced it under slow builds → UAF.
- Lower per-test event counts (50 -> 10) so a handful of real HTTP
deliveries finish well within the timeout under instrumentation.
Queue-cap drop coverage: make_RPCSub gains a defaulted maxQueueSize
parameter (production default unchanged at 16384) so a test can set a
tiny cap and exercise the drop path by out-pacing delivery — enqueue is
microsecond-fast while each delivery is a full round-trip — instead of
queueing 16384 events. New testQueueCapDrops confirms some events are
delivered and some dropped.
Raises RPCSub.cpp patch coverage 62% -> 87% (overall diff 65% -> 87%);
the only remaining uncovered lines are the defensive sendThread
exception catches.
RPCSub_test (new): the webhook delivery flow control + EOF handling had
zero automated coverage. Three cases drive events through a real local
HTTP endpoint: delivery, delivery continues despite HTTP 500 responses
(the #6341 stall, exercised via EOF-delimited error replies), and
sending restarts after the queue drains (mSending reset).
HTTPClient_test mock fixes:
- heldSockets_: 'sendResponse=false' now genuinely holds the connection
open instead of dropping the only socket shared_ptr (which closed it).
testCleanupAfterTimeout now actually exercises the deadline path.
- await-client-EOF on Content-Length responses: the mock decrements its
connection count only when the CLIENT closes its socket, so
activeConnectionCount() reflects client-side FD release rather than the
server's own lifecycle — giving testGetSelfReferenceCleanup real teeth.
- poll helpers replace fixed sleeps for the cross-thread count checks.
get() bound shared_from_this() into mBuild (a member), forming a
this -> mBuild -> shared_ptr<this> cycle that never broke after the
request completed — leaking the HTTPClientImp and its socket FD. mBuild
is only ever invoked from handleRequest(), which always runs inside an
async handler already holding a shared_from_this(), so a non-owning
this is safe and lets the object be destroyed once the request finishes.
Pre-existing (also present upstream in rippled); only the fetch path
uses get(), not the webhook delivery path.
Revert the eof->success translation from the previous commit. The
upstream rippled #6344 deliberately keeps forwarding ecResult as-is;
translating eof to success would report a truncated Content-Length
response (server closes before sending the promised N body bytes) as a
successful read and suppress HTTPClient::get() multi-site fallback.
Keeps the structural simplification (collapsed branches, early return).
Backports the robustness changes that landed during review of the
upstream rippled PR (XRPLF/rippled#6344) but never made it into this
branch, plus the applicable Copilot suggestions.
RPCSub::sendThread:
- Wrap the whole dispatch loop in try/catch so mSending is reset under
the lock on EVERY exit path (drain, dispatch throw, run() throw).
Previously a throw from the lock-guarded dispatch block escaped
sendThread() without clearing mSending, re-introducing the original
stall (issue #6341) where send() never starts another job.
- Bail out (reset mSending + return) on an io_service.run() exception
instead of silently looping.
RPCSub::send:
- Rate-limit the queue-full drop warning (once per dropLogInterval) so
a persistently behind endpoint can't flood the log; still advance
mSeq on every drop so consumers detect the gap.
HTTPClient::handleData:
- Collapse the identical EOF/non-EOF completion branches into one and
early-return on read error (per maintainer review).
- Report success (not eof) to invokeComplete on the EOF-delimited path
so callers don't treat a complete response as a failure.
Resolves conflicts from the src/ripple -> src/xrpld + cmake restructure:
- src/xrpld/net/RPCSub.h: take dev's xrpld/ include paths, keep our
removal of the now-unused boost/asio/io_service.hpp include
- Builds/CMake/RippledCore.cmake: accept dev's deletion; tests are now
auto-discovered via GLOB_RECURSE over src/test, so the explicit
HTTPClient_test.cpp listing is obsolete
- Builds/levelization/results/ordering.txt: regenerated via
levelization.py (adds test.net > {xrpl.basics, xrpld.net, test.toplevel})
- src/test/net/HTTPClient_test.cpp: update ripple/ includes to xrpl//xrpld/
