diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 75ccf6528f5..a05d19db1f2 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -595,6 +595,16 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r // data. The original data will go out of scope when WriteReply() returns. // This is analogous to the memcpy() in libevent's evbuffer_add() m_client->m_send_buffer.insert(m_client->m_send_buffer.end(), reply_body.begin(), reply_body.end()); + + // If the buffer already held data, the I/O thread is (or soon will be) + // draining it, so flag that there is more data to send. This must happen + // while holding m_send_mutex and while the buffer is known non-empty: + // setting m_send_ready after releasing the lock would race with the I/O + // thread draining the buffer to empty and clearing m_send_ready in + // between, leaving m_send_ready set on an empty buffer. The I/O loop would + // then only ever poll the socket for writeability, never read the client's + // next request, and wedge the connection. + if (!send_buffer_was_empty) m_client->m_send_ready = true; } LogDebug( @@ -611,10 +621,6 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r // of waiting for the next iteration of the I/O loop. if (send_buffer_was_empty) { m_client->MaybeSendBytesFromBuffer(); - } else { - // Inform HTTPServer I/O that data is ready to be sent to this client - // in the next loop iteration. - m_client->m_send_ready = true; } // Signal to the I/O loop that we are ready to handle the next request. @@ -935,7 +941,12 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const // Check if client is ready to send data. Don't try to receive again // until the send buffer is cleared (all data sent to client). - Sock::Event event = (http_client->m_send_ready ? Sock::SendEvent : Sock::RecvEvent); + // Keep this as a separate critical section from the m_sock_mutex one above: + // never hold m_sock_mutex and m_send_mutex at the same time here. + // MaybeSendBytesFromBuffer() locks m_send_mutex then m_sock_mutex, so nesting + // them in the opposite order here would risk a lock-order inversion deadlock. + const bool send_ready{WITH_LOCK(http_client->m_send_mutex, return http_client->m_send_ready;)}; + Sock::Event event = (send_ready ? Sock::SendEvent : Sock::RecvEvent); io_readiness.events_per_sock.emplace(sock, Sock::Events{event}); io_readiness.httpclients_per_sock.emplace(sock, http_client); } diff --git a/src/httpserver.h b/src/httpserver.h index 792b3690b92..9031eb61e0a 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -483,11 +483,14 @@ public: /// @} /** - * Set true by worker threads after writing a response to m_send_buffer. - * Set false by the HTTPServer I/O thread after flushing m_send_buffer. - * Checked in the HTTPServer I/O loop to avoid locking m_send_mutex if there's nothing to send. - */ - std::atomic_bool m_send_ready{false}; + * Set true by worker threads after writing a response to m_send_buffer. + * Set false by the HTTPServer I/O thread after flushing m_send_buffer. + * Checked in the HTTPServer I/O loop to decide whether to poll the socket for + * writeability or readability. + * Guarded by m_send_mutex so it stays consistent with m_send_buffer's emptiness: + * the two must always be updated together under the same lock. + */ + bool m_send_ready GUARDED_BY(m_send_mutex){false}; /** * Mutex that serializes the Send() and Recv() calls on `m_sock`. Reading