Skip to content
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

rpc: Fix data race (UB) in InterruptRPC() #14993

Merged
merged 1 commit into from Dec 19, 2018

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Dec 18, 2018

Fix data race (UB) in InterruptRPC().

Before:

$ ./configure --with-sanitizers=thread
$ make
$ test/functional/test_runner.py feature_shutdown.py
…
SUMMARY: ThreadSanitizer: data race rpc/server.cpp:314 in InterruptRPC()
…
ALL                 | ✖ Failed  | 2 s (accumulated)

After:

$ ./configure --with-sanitizers=thread
$ make
$ test/functional/test_runner.py feature_shutdown.py
…
ALL                 | ✓ Passed  | 3 s (accumulated)
@laanwj
Copy link
Member

@laanwj laanwj commented Dec 18, 2018

utACK 10f8244a777392f87c2671d743b14201d24351be

I don't think this can actually result in a data race but an atomic is certainly more appropriate here.

src/rpc/server.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Member

@promag promag commented Dec 18, 2018

utACK 9ed8713.

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Remove from the suppressions file?

src/rpc/server.cpp Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Dec 18, 2018

@MarcoFalke Done! Please re-review :-)

@promag
Copy link
Member

@promag promag commented Dec 18, 2018

utACK 6c10037.

@laanwj laanwj merged commit 6c10037 into bitcoin:master Dec 19, 2018
2 checks passed
laanwj added a commit that referenced this issue Dec 19, 2018
6c10037 rpc: Fix data race (UB) in InterruptRPC() (practicalswift)

Pull request description:

  Fix data race (UB) in `InterruptRPC()`.

  Before:

  ```
  $ ./configure --with-sanitizers=thread
  $ make
  $ test/functional/test_runner.py feature_shutdown.py
  …
  SUMMARY: ThreadSanitizer: data race rpc/server.cpp:314 in InterruptRPC()
  …
  ALL                 |  Failed  | 2 s (accumulated)
  ```

  After:

  ```
  $ ./configure --with-sanitizers=thread
  $ make
  $ test/functional/test_runner.py feature_shutdown.py
  …
  ALL                 | ✓ Passed  | 3 s (accumulated)
  ```

Tree-SHA512: b139ca1a0480258f8caa7730cabd7783a821d906630f51487750a6b15b7842675ed679747e1ff1bdade77d248807e9d77bae7bb88da54d1df84a179cd9b9b987
@Sjors
Copy link
Member

@Sjors Sjors commented Dec 21, 2018

I'm still getting feature_shutdown.py failures on Travis, see e.g. https://travis-ci.org/bitcoin/bitcoin/jobs/470997593#L3104

@promag
Copy link
Member

@promag promag commented Dec 21, 2018

@Sjors that's fixed by #14958 which depends on #14982.

@markblundeberg
Copy link

@markblundeberg markblundeberg commented Feb 6, 2020

(introduced in #14670)

markblundeberg referenced this issue in codablock/dash Feb 7, 2020
28479f9 qa: Test bitcond shutdown (João Barbosa)
8d3f46e http: Remove timeout to exit event loop (João Barbosa)
e98a9ee http: Remove unnecessary event_base_loopexit call (João Barbosa)
6b13580 http: Unlisten sockets after all workers quit (João Barbosa)
18e9685 http: Send "Connection: close" header if shutdown is requested (João Barbosa)
02e1e4e rpc: Add wait argument to stop (João Barbosa)

Pull request description:

  Fixes bitcoin#11777. Reverts bitcoin#11006. Replaces bitcoin#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

Tree-SHA512: 4dac1e86abe388697c1e2dedbf31fb36a394cfafe5e64eadbf6ed01d829542785a8c3b91d1ab680d3f03f912d14fc87176428041141441d25dcb6c98a1e069d8
@practicalswift practicalswift deleted the rpc-datarace branch Apr 10, 2021
LarryRuane added a commit to LarryRuane/zcash that referenced this issue Apr 29, 2021
LarryRuane added a commit to LarryRuane/zcash that referenced this issue Jun 1, 2021
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 24, 2021
Already fixed here:
41190e9 ("Fix data races triggered by functional tests. (dashpay#4247)", 2021-07-26)

6c10037 rpc: Fix data race (UB) in InterruptRPC() (practicalswift)

Pull request description:

  Fix data race (UB) in `InterruptRPC()`.

  Before:

  ```
  $ ./configure --with-sanitizers=thread
  $ make
  $ test/functional/test_runner.py feature_shutdown.py
  …
  SUMMARY: ThreadSanitizer: data race rpc/server.cpp:314 in InterruptRPC()
  …
  ALL                 |  Failed  | 2 s (accumulated)
  ```

  After:

  ```
  $ ./configure --with-sanitizers=thread
  $ make
  $ test/functional/test_runner.py feature_shutdown.py
  …
  ALL                 | ✓ Passed  | 3 s (accumulated)
  ```

Tree-SHA512: b139ca1a0480258f8caa7730cabd7783a821d906630f51487750a6b15b7842675ed679747e1ff1bdade77d248807e9d77bae7bb88da54d1df84a179cd9b9b987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants