Page MenuHomePhabricator

Merge #14670: http: Fix HTTP server shutdown
ClosedPublic

Authored by markblundeberg on Feb 6 2020, 03:45.

Details

Summary

PR14670 backport. Completes T537.

28479f926f21f2a91bec5a06671c60e5b0c55532 qa: Test bitcond shutdown (João Barbosa)
8d3f46ec3938e2ba17654fecacd1d2629f9915fd http: Remove timeout to exit event loop (João Barbosa)
e98a9eede2fb48ff33a020acc888cbcd83e24bbf http: Remove unnecessary event_base_loopexit call (João Barbosa)
6b13580f4e3842c11abd9b8bee7255fb2472b6fe http: Unlisten sockets after all workers quit (João Barbosa)
18e968581697078c36a3c3818f8906cf134ccadd http: Send "Connection: close" header if shutdown is requested (João Barbosa)
02e1e4eff6cda0bfc24b455a7c1583394cbff6eb rpc: Add wait argument to stop (João Barbosa)

Pull request description:

Fixes #11777. Reverts #11006. Replaces #13501.

With this change the HTTP server will exit gracefully, meaning that all requests will finish processing and sending the response, even if this means to wait more than 2 seconds (current time allowed to exit the event loop).

Another small change is that connections are accepted even when the server is stopping, but HTTP requests are rejected. This can be improved later, especially if chunked replies are implemented.

Briefly, before this PR, this is the order or events when a request arrives (RPC `stop`):
 1. `bufferevent_disable(..., EV_READ)`
 2. `StartShutdown()`
 3. `evhttp_del_accept_socket(...)`
 4. `ThreadHTTP` terminates (event loop exits) because there are no active or pending events thanks to 1. and 3.
 5. client doesn't get the response thanks to 4.

This can be verified by applying
```diff
     // Event loop will exit after current HTTP requests have been handled, so
     // this reply will get back to the client.
     StartShutdown();
+    MilliSleep(2000);
     return "Bitcoin server stopping";
 }
```
and checking the log output:
```
    Received a POST request for / from 127.0.0.1:62443
    ThreadRPCServer method=stop user=__cookie__
    Interrupting HTTP server
**  Exited http event loop
    Interrupting HTTP RPC server
    Interrupting RPC
    tor: Thread interrupt
    Shutdown: In progress...
    torcontrol thread exit
    Stopping HTTP RPC server
    addcon thread exit
    opencon thread exit
    Unregistering HTTP handler for / (exactmatch 1)
    Unregistering HTTP handler for /wallet/ (exactmatch 0)
    Stopping RPC
    RPC stopped.
    Stopping HTTP server
    Waiting for HTTP worker threads to exit
    msghand thread exit
    net thread exit

    ... sleep 2 seconds ...

    Waiting for HTTP event thread to exit
    Stopped HTTP server
```

For this reason point 3. is moved right after all HTTP workers quit. In that moment HTTP replies are queued in the event loop which keeps spinning util all connections are closed. In order to trigger the server side close with keep alive connections (implicit in HTTP/1.1) the header `Connection: close` is sent if shutdown was requested. This can be tested by
```
bitcoind -regtest
nc localhost 18443
POST / HTTP/1.1
Authorization: Basic ...
Content-Type: application/json
Content-Length: 44

{"jsonrpc": "2.0","method":"stop","id":123}
```

Summing up, this PR:
 - removes explicit event loop exit — event loop exits once there are no active or pending events
 - changes the moment the listening sockets are removed — explained above
 - sends header `Connection: close` on active requests when shutdown was requested which is relevant when it's a persistent connection (default in HTTP 1.1) — libevent is aware of this header and closes the connection gracefully
 - removes event loop explicit break after 2 seconds timeout

Backport notes:

This slows down the --usecli tests considerably! See:
https://github.com/bitcoin/bitcoin/issues/15309
There is also a test race in the new test, will backport separately:
https://github.com/bitcoin/bitcoin/pull/14958/files

Fixes in D5171 D5174 will be landed immediately after this one.

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr14670
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9289
Build 16515: Default Diff Build & Tests
Build 16514: arc lint + arc unit

Event Timeline

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

This revision is now accepted and ready to land.Feb 6 2020, 07:42