http: Make server shutdown more robust #31929
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31929. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Assertion failed: (evb), function WriteReply, file httpserver.cpp, line 682.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/http_request/83149a7e3abd937c7bc67fa55b9a4fc9338e3d8c"
⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/http_request')] |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
hodlinator
force-pushed
the
2025/02/stop_http_robust
branch
from
ef290c0 to
24558c2
Compare
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
The fuzz-failure was me underestimating a default-initialized argument. Fixed in 2nd push.
|
From what i remember this was extrememly hard to get right with libevent (or at least our use of it), without leaking anything or running into race conditions. There have been a few tries to fix the behavior over the years, but they all were different compromises, adding ever more compexity, and often introducing new problems. As it seems to be where things are heading anyway (#32061) maybe it's better to focus on replacing libevent and its webserver framework wholesale. |
|
Thank you for providing your perspective @laanwj! In working on this PR, I've also gained an appreciation for getting rid of libevent. However, I'm not sure when that will happen, maybe not until post-30.0. In the meantime we are seeing issues on CI such as #31894 (analysis). What this PR is doing is adding our own request ids to track which HTTP request never finishes. When we have stuck HTTP requests, it also makes us abort the process with a clear error after 10min30s instead of waiting for the test framework to time out after 40min. On top of that it also fixes an intermittent leak, right after we've joined on the thread that is the only one to sometimes perform deletion. Have come across #26742 and #19420 while working on this, so have seen some of the struggle. Don't have an overall grasp on how frequent issues like 31894 are on CI (or in the wild), it's fair that that frequency should influence the priority of this compared to other work. |
Could aid debugging when an HTTP request gets stuck at shutdown.
hodlinator
force-pushed
the
2025/02/stop_http_robust
branch
from
24558c2 to
d8c8cc1
Compare
Could happen when libevent HTTP connection doesn't complete for some reason. Output early warning to give clue about what's up. Prior check+log message before even starting to wait was a bit too eager to hint at something being wrong.
There is a race between ThreadHTTP exiting and our queuing of the freeing-event (event_base_once).
Additional free was useful in a majority of runs - 709/962.
Memory leaks during shutdown are not a big issue (unless growing unbounded) as the OS will clean up after the process, but still nice to tidy up.
Can be reproduced by on this commit by running something like:
```shell
#!/usr/bin/env bash
mv ~/.bitcoin/debug.log ~/.bitcoin/debug.log.old
# Allows overriding total through `$ ./repeat.sh <TOTAL>`
TOTAL=${1:-100}
for ((i=1; i<=$TOTAL; i++)); do
echo "Repetition $i/$TOTAL"
./build/bin/bitcoind -daemonwait -noconnect -debug=libevent -debug=http -debug=rpc
./build/bin/bitcoin-cli stop
if [ $? -ne 0 ]; then
echo "Exit code $?"
exit
fi
while [ -f "$HOME/.bitcoin/bitcoind.pid" ]; do
echo "PID file still exists"
sleep 0.1
done
done
echo "Leak averted in $(grep "Freeing eventHTTP-event" ~/.bitcoin/debug.log -c)/$(grep "Bitcoin Core version" ~/.bitcoin/debug.log -c) cases."
```
hodlinator
force-pushed
the
2025/02/stop_http_robust
branch
from
d8c8cc1 to
aa11ee5
Compare
|
Spent some time revisiting and polishing this since 24558c2cf18210f46d6e2fadf0c5c5912f4b8e10.
|
|
Was worried that libevent would clean up the memory leak I thought was fixed by aa11ee5 later during shutdown, so verified that that libevent does no such thing using Valgrind: master...hodlinator:bitcoin:2025/02/stop_http_robust.valgrind |
4 participants
Should help debug #31894.