This prevents a losing race condition that could prevent the server from reading any more requests from an HTTP client.
Found and reported by the fuzzing department: https://github.com/dergoegge/bitcoin/commit/7fe5f54497c86f216b619e340939447504e87dcb
The Race:
A connected socket can either be written to or read from based on the result of GenerateWaitSockets(). That method checks the HTTPRemoteClient flag m_send_ready. If it's true the implication is that there is data in the client's send buffer ready to go. Once that data is sent and the buffer is empty, MaybeSendBytesFromBuffer() sets it false again.
The sad case was when a worker thread calling WriteReply() adds data to the send buffer, but before it sets m_send_ready to true, the I/O thread sends that data and empties the buffer. With the buffer unexpectedly empty, WriteReply() sets m_send_ready to true.
The effect of this is that the socket will stay in "write" mode with nothing to write. With nothing to write, MaybeSendBytesFromBuffer() never sets it back to false and the socket is stuck forever.
The Fix:
Simply move m_send_ready = true inside the block of WriteReply() where m_send_mutex is still held. This prevents the I/O thread from emptying the send buffer while the worker thread is setting the flag.
Testing:
To observe the race condition, revert the first commit "http: prevent race condition between worker thread and I/O thread" and run the unit test from the remainder of the branch. I like to see the logs:
`test_bitcoin --log_level=all --run_test=httpserver_tests -- --printtoconsole --debug=http --debug=lock'
The test will fail with a small probability. The socket will get stuck and the test will abort after a 60 second timeout. To garuntee the race condition loses and fail the test every time, slow down WriteReply() in the worker thread:
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 99e30ff663..b0c7b516d8 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -614,6 +614,7 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> r
} else {
// Inform HTTPServer I/O that data is ready to be sent to this client
// in the next loop iteration.
+ std::this_thread::sleep_for(500ms);
m_client->m_send_ready = true;
}
With the first commit (the fix) back in place, slowing down the worker thread like this won't fail the test.
Bonus:
The unit test is spread over three commits. First, a method of the socket testing setup is templated so a mock socket that intentionally raises an error can be inserted. The unit test added in that commit covers a race condition that was fixed in #35182 in response to https://github.com/bitcoin/bitcoin/pull/35182/changes#r3358889539 so we get the added benefit of covering an error path, and guaranteeing coverage of both "optimistic send" (directly from worker thread) and regular send (from a tick in the I/O loop thread).
The next commit adds a worker thread to the unit test, at which point a race condition is possible but very unlikely because all requests are sent at once. Finally, we spread out the requests in the top commit and make the race condition much easier to catch.
