one idea i went back and forth on when looking at the send-buffer throttle: instead of a single 32 MiB cutoff for the send-buffer throttle, this uses two marks: a high-water mark (HWM) that enables throttling, and a lower-water mark (LWM) that disables it
the motivation is to avoid flapping around the boundary. with a single cutoff, a connection sitting near 32 MiB can repeatedly cross above/below it as a few bytes drain and refill. that means we keep changing whether reads are allowed even though the connection's behaviour has not meaningfully changed. also, the fact that the send buffer is not fixed in size so it is continuously changing as the server queues more responses or the remote peer socket drains data
with HWM/LWM, once we enter the congested state, we stay there until the buffer drains meaningfully below the LWM. that adds m_send_congested as a new piece of state on the client that can persist/changed across I/O ticks
upsides of HWM/LWM over the current single-boundary approach:
- no thrash for a connection parked near the boundary therefore fewer wake-ups on the I/O thread
- makes the congestion state explicit and deterministic
- gives us clear transitions to test: HWM turns throttling on, LWM turns it off
upsides of the current single-boundary approach over HWM/LWM:
- two constants instead of one, so there's another value to justify/tune
- adds per-connection state that we now need to keep correct
- a client that recovers quickly (crosses back under 32MiB right away) resumes immediately, instead of having to drain further down to LWM first. so, it is strictly better for a legitimate-but-bursty client's latency
[!NOTE]
those are some on the pros and cons i'd quickly think of. you'd think of more
genuinely unsure which way nets out better here therefore i wanted to flag this as its own discussion point. curious if others think the hysteresis is worth the extra state, or if we're solving a thrash problem that is less likely to actually show up in practice
<details>
<summary>a quick rough idea of how i think it could be implemented
</summary>
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index a29109c211..38282c475d 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -632,6 +632,20 @@ void HTTPRemoteClient::Send(const HTTPResponse& res, std::span<const std::byte>
// 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_send_ready = true;
+
+ // Latch congestion on once we cross the high watermark. This is only
+ // ever set here; it's only cleared in MaybeSendBytesFromBuffer()
+ // once the buffer has drained back below the low watermark, which
+ // is what gives the hysteresis its effect.
+ if (!m_send_congested && m_send_buffer.size() > SEND_BUF_HIGH_WATERMARK) {
+ m_send_congested = true;
+ LogDebug(
+ BCLog::HTTP,
+ "Client %s (id=%llu) is send-congested (%d bytes queued) throttling reads/dispatch",
+ m_origin,
+ m_id,
+ m_send_buffer.size());
+ }
}
LogDebug(
@@ -1005,7 +1019,8 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const
// full, TryReadRequest() holds a completed request back from a
// worker, so nothing new is read until send has drained.
// 2. Else, m_req is incomplete and needs more data, or there is no
- // m_req at all and the recv buffer is empty -> Recv
+ // m_req at all and the recv buffer is empty, provided the send buffer
+ // is not congested -> Recv
// 3. Else (no parse in progress, leftover bytes in m_recv_buffer) -> 0
// Stay in the I/O map so TryReadRequest() drains the buffer first.
// Extra pipelined data waits in the kernel socket buffer
@@ -1021,8 +1036,8 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const
Sock::Event event{0};
if (http_client->ReadyToSend()) {
event = Sock::SendEvent;
- } else if (http_client->GetRequest() != nullptr || http_client->ReceiveBufferEmpty()) {
- // Mid-parse (need more bytes) or buffer empty.
+ } else if (!http_client->IsSendCongested() && (http_client->GetRequest() != nullptr || http_client->ReceiveBufferEmpty())) {
+ // Mid-parse (need more bytes) or buffer empty - but only when the send buffer is not congested.
event = Sock::RecvEvent;
}
@@ -1067,6 +1082,12 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
// loop iteration.
if (client->m_req_busy) return nullptr;
+ // If this client's send buffer is congested, don't read or parse
+ // anything new from it either. This is what stops the server from
+ // packing more responses into m_send_buffer for a client that has
+ // stopped draining its socket
+ if (client->IsSendCongested()) return nullptr;
+
if (!client->m_req) {
client->m_req = std::make_unique<HTTPRequest>(client);
}
@@ -1106,10 +1127,10 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
// the server from reading any more data from this client until they
// drain their end of the socket, and prevents the server from packing
// more responses into the send buffer.
- const size_t buffer_used{WITH_LOCK(
- client->m_send_mutex,
- return client->m_send_buffer.size();)};
- if (buffer_used > MAX_BODY_SIZE) return nullptr;
+ // const size_t buffer_used{WITH_LOCK(
+ // client->m_send_mutex,
+ // return client->m_send_buffer.size();)};
+ // if (buffer_used > MAX_BODY_SIZE) return nullptr;
LogDebug(
BCLog::HTTP,
"Received a %s request for %s from %s (id=%llu)",
@@ -1293,6 +1314,16 @@ bool HTTPRemoteClient::MaybeSendBytesFromBuffer()
m_send_buffer.erase(m_send_buffer.begin(),
m_send_buffer.begin() + bytes_sent);
+ // Only clear congestion once drained below the low watermark
+ if (m_send_congested && m_send_buffer.size() < SEND_BUF_LOW_WATERMARK) {
+ m_send_congested = false;
+ LogDebug(
+ BCLog::HTTP,
+ "Client %s (id=%llu) send buffer drained, resuming reads/dispatch",
+ m_origin,
+ m_id);
+ }
+
LogDebug(
BCLog::HTTP,
"Sent %d bytes to client %s (id=%llu)",
diff --git a/src/httpserver.h b/src/httpserver.h
index dae59a9443..f043dbe655 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -78,7 +78,6 @@ inline constexpr size_t MIN_REQUEST_LINE_LENGTH = std::string_view("GET / HTTP/1
inline constexpr size_t MAX_HEADERS_SIZE{8192};
//! Maximum size of an HTTP request body received from a client.
-//! Also used to limit data queued for sending back to client.
inline constexpr uint64_t MAX_BODY_SIZE{32_MiB};
//! Thrown when a request body exceeds MAX_BODY_SIZE (or *will* exceed, in chunked transfer)
@@ -86,6 +85,17 @@ inline constexpr uint64_t MAX_BODY_SIZE{32_MiB};
struct ContentTooLargeError : std::runtime_error {
using std::runtime_error::runtime_error;
};
+
+//! Send-buffer backpressure thresholds, deliberately kept separate from
+//! MAX_BODY_SIZE: that constant bounds request bodies we accept, this pair
+//! bounds queued response bytes we're willing to hold for a slow/stalled
+//! reader. Once a client's queued send data exceeds the high watermark it is
+//! marked congested (see HTTPRemoteClient::IsSendCongested()); it stays
+//! congested until the buffer drains back below the low watermark, so a
+//! connection sitting near the boundary does not flap in and out of
+//! throttling on every I/O loop tick.
+inline constexpr size_t SEND_BUF_LOW_WATERMARK{16_MiB};
+inline constexpr size_t SEND_BUF_HIGH_WATERMARK{32_MiB};
} // namespace bitcoin_http
class HTTPHeaders
@@ -505,6 +515,17 @@ public:
bool ReadyToSend() const EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex) { return WITH_LOCK(m_send_mutex, return m_send_ready;); }
bool ReceiveBufferEmpty() const { return m_recv_buffer.empty(); }
+ /**
+ * True once queued response bytes for this client have crossed
+ * SEND_BUFFER_HIGH_WATERMARK, and stays true until they drain below
+ * SEND_BUFFER_LOW_WATERMARK (see the constants' doc comment). Checked
+ * identically by HTTPServer::GenerateWaitSockets() (to stop reading more
+ * request data from the connection's socket) and TryReadRequest() (to stop
+ * dispatching this client's requests to a worker), so both consult one
+ * source of truth.
+ */
+ bool IsSendCongested() const EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex) { return WITH_LOCK(m_send_mutex, return m_send_congested;); }
+
void Send(const HTTPResponse& res, std::span<const std::byte> reply_body, bool keep_alive) EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex);
void Receive() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex);
@@ -514,7 +535,11 @@ public:
* Try to read an HTTPRequest from a client's receive buffer.
* Only complete requests are returned, incomplete requests are
* left in the buffer to wait for more data. Some read errors
- * will mark this client for disconnection.
+ * will mark this client for disconnection. Also returns nullptr,
+ * without reading or parsing anything, while the client is
+ * send-congested -- this is what stops a misbehaving client that
+ * is not draining its socket from causing m_send_buffer to grow
+ * without bound.
*/
static std::unique_ptr<HTTPRequest> TryReadRequest(const std::shared_ptr<HTTPRemoteClient>& client) EXCLUSIVE_LOCKS_REQUIRED(!client->m_send_mutex);
@@ -580,6 +605,12 @@ private:
/// @{
mutable Mutex m_send_mutex;
std::vector<std::byte> m_send_buffer GUARDED_BY(m_send_mutex);
+
+ //! Hysteretic congestion flag;. Only ever set in
+ //! Send() (where m_send_buffer grows) and only ever cleared in
+ //! MaybeSendBytesFromBuffer() (where it shrinks), so it always reflects
+ //! the buffer's size as of the last time it changed.
+ bool m_send_congested GUARDED_BY(m_send_mutex){false};
/// @}
/**
</details>
any thoughts ?
