-
Notifications
You must be signed in to change notification settings - Fork 38.7k
http: Release server before waiting for event base loop exit #15363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@sdaftuar regarding #15305, if you cherry pick commit to your branch you'll see the test runs in a couple of seconds. Also, with the following patch: --- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -465,6 +465,7 @@ void StopHTTPServer()
boundSockets.clear();
if (eventBase) {
LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n");
+ event_base_dump_events(eventBase, stderr);
// Exit the event loop as soon as there are no active events.
event_base_loopexit(eventBase, nullptr);
threadHTTP.join();and running test/functional/feature_abortnode.py --nocleanupyou will see (mind the path): cat /var/folders/1v/8_69hby54nj2k3n6fywt44x80000gn/T/bitcoin_func_test_o4hopx4l/node0/stderr/tmp*
: Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
Inserted events:
Active events:
Error: Error: A fatal internal error occurred, see debug.log for details
Inserted events:
0x7fb93d44b560 [fd 14] Read Persist Timeout=1549549849.031699
Active events: |
|
Improves #15309. |
edd9829 to
a0f6915
Compare
|
#15350 was merged, updated OP. |
|
We just can't keep "fixing" this, can we? |
|
This is a different issue, idle connections are stalling shutdown. We could decrease the timeout duration but I don't think that's a fix. This is incomplete. |
The libevent HTTP interface is kind of crazy, so I don't think it's surprising that fixing this requires some trial and error. But it would be good to have tests for each issue that gets fixed to avoid going in circles. If it would be possible to specify what should happen on shutdown for connections in different states (close headers? truncated responses? timeouts?) and write a more comprehensive client test that would of course be great, too. |
|
@ryanofsky agree, and test/functional/feature_shutdown.py was added with that in mind. I'll add test for this change. BTW, I think the case introduced by @sdaftuar in #15305 is new because the shutdown isn't triggered by |
|
Not sure if we should pull this into 0.18 at this point. Removing for now |
|
@MarcoFalke sure I agree. The issue is on shutdown so it shouldn't bother too much to wait for connections to timeout - beside our functional tests. |
a0f6915 to
a856a30
Compare
|
What is the status of this? It has nothing to do with #16405, right? |
@MarcoFalke |
|
Currently in master 3a3d8b8 It takes 30 seconds to timeout and only then the event base quits. This PR fixes it but IMO in a wrong way. I will update shortly. |
c51a4ed to
79da6bd
Compare
|
When I run the functional test suite on a RAM disk it's bottlenecked by the slowest running tests. I looked at some of those tests and saw they were stuck on Cherry-picking this commit speeds up |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
Now |
|
For now closing in favor of #19420. |
Call
evhttp_freein the event loop to trigger server shutdown, in particular to close connections.