Skip to content

Ensure that http event loop is empty before the loop exit#13485

Closed
ken2812221 wants to merge 1 commit intobitcoin:masterfrom
ken2812221:http_shutdown
Closed

Ensure that http event loop is empty before the loop exit#13485
ken2812221 wants to merge 1 commit intobitcoin:masterfrom
ken2812221:http_shutdown

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Jun 16, 2018 •
edited
Loading

Close #11777
This also revert #11006, since event_base_loopexit seems to drop unstarted event. So shutdown might take longer.

@ken2812221 ken2812221 mentioned this pull request Jun 16, 2018
Closed
@ken2812221 ken2812221 changed the title Ensure that event loop is empty before the loop exit Ensure that http event loop is empty before the loop exit Jun 17, 2018
@promag
Copy link
Contributor

promag commented Jun 17, 2018

This should be simple, I don't think libevent would force such workarounds.

since event_base_loopexit seems to drop unstarted event

According to the documentation it ignores pending events but active events are handled.

And looks like sending the reply is an active event ():

bitcoin/src/httpserver.cpp

Lines 583 to 608 in d6cf4bd

void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
{
assert(!replySent && req);
// Send event to main http thread to send reply message
struct evbuffer* evb = evhttp_request_get_output_buffer(req);
assert(evb);
evbuffer_add(evb, strReply.data(), strReply.size());
auto req_copy = req;
HTTPEvent* ev = new HTTPEvent(eventBase, true, [req_copy, nStatus]{
evhttp_send_reply(req_copy, nStatus, nullptr, nullptr);
// Re-enable reading from the socket. This is the second part of the libevent
// workaround above.
if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) {
evhttp_connection* conn = evhttp_request_get_connection(req_copy);
if (conn) {
bufferevent* bev = evhttp_connection_get_bufferevent(conn);
if (bev) {
bufferevent_enable(bev, EV_READ | EV_WRITE);
}
}
}
});
ev->trigger(nullptr);
replySent = true;
req = nullptr; // transferred back to main thread
}

bitcoin/src/httpserver.cpp

Lines 519 to 525 in d6cf4bd

void HTTPEvent::trigger(struct timeval* tv)
{
if (tv == nullptr)
event_active(ev, 0, 0); // immediately trigger event in main thread
else
evtimer_add(ev, tv); // trigger after timeval passed
}

Maybe there is a problem in when the the event is activ'ated?

@ken2812221
Copy link
Contributor Author

ken2812221 commented Jun 17, 2018 •
edited
Loading

Thatevent_active could be called after threadHTTP exited and the event wouldn't get called in rare case. This can be tested by adding sleep(1) between StartShutdown() and return statement.

bitcoin/src/rpc/server.cpp

Lines 229 to 240 in d6cf4bd

UniValue stop(const JSONRPCRequest& jsonRequest)
{
// Accept the deprecated and ignored 'detach' boolean argument
if (jsonRequest.fHelp || jsonRequest.params.size() > 1)
throw std::runtime_error(
"stop\n"
"\nStop Bitcoin server.");
// Event loop will exit after current HTTP requests have been handled, so
// this reply will get back to the client.
StartShutdown();
return "Bitcoin server stopping";
}

@promag
Copy link
Contributor

promag commented Jun 17, 2018

@ken2812221 so instead it should make sure the reply is handled before event_base_loopexit is called?

@ken2812221
Copy link
Contributor Author

ken2812221 commented Jun 17, 2018

should make sure the reply is handled before threadHTTP loop exit because the loop can exit earlier than event_base_loopexit called. However I have no idea how to do that.

@promag promag mentioned this pull request Jun 18, 2018
Closed
@ken2812221 ken2812221 closed this Jun 18, 2018
@ken2812221 ken2812221 deleted the http_shutdown branch June 18, 2018 02:37
@ken2812221 ken2812221 mentioned this pull request Jun 21, 2018
Closed
@maflcko
Copy link
Member

maflcko commented Aug 8, 2018

@ken2812221 Are you still working on this?

@ken2812221 ken2812221 restored the http_shutdown branch August 9, 2018 00:04
@ken2812221
Copy link
Contributor Author

ken2812221 commented Aug 9, 2018

@MarcoFalke If necessary, I can work on this again.

@ken2812221 ken2812221 reopened this Aug 9, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 9, 2018 •
edited
Loading

Reviewers, 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.

@maflcko
Copy link
Member

maflcko commented Aug 10, 2018

Tested 4e79d3e on current master and could run the tests for 24 hours in a loop. (Which is not possible on current master)

@ken2812221 ken2812221 closed this Aug 21, 2018
@ken2812221 ken2812221 deleted the http_shutdown branch August 21, 2018 00:59
@ken2812221 ken2812221 restored the http_shutdown branch August 24, 2018 13:53
@ken2812221 ken2812221 deleted the http_shutdown branch September 20, 2018 10:00
@ken2812221 ken2812221 restored the http_shutdown branch October 2, 2018 21:16
@ken2812221 ken2812221 reopened this Oct 2, 2018
@ken2812221 ken2812221 closed this Oct 13, 2018
@ken2812221 ken2812221 deleted the http_shutdown branch October 13, 2018 04:13
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[qa] Tests occasionally raise http.client.RemoteDisconnected or BadStatusLine

4 participants

@ken2812221 @promag @maflcko @DrahtBot