Fix 5 bugs found via wrk stress testing the HTTP proxy
1. tcp_write infinite loop: When the output byte queue enters an error
state (buffer full), tcp_write would loop forever because
byte_queue_write_buf returns {NULL,0} without advancing. Break out
of the loop when the write buffer is empty.
2. Proxy ignores async return values: toastyfs_async_get/put/delete
can fail (return -1) but the proxy unconditionally set the operation
to STARTED, causing it to wait forever for a result that never comes.
Now checks return values and responds with 500 on failure.
3. Use-after-free on closed connections: When a client disconnects while
its request is queued as PENDING, the HTTP_Conn is freed but the proxy
still holds stale pointers. Added http_response_builder_is_valid() to
detect and discard operations for dead connections.
4. Wrong HTTP status codes: The proxy returned 500 for all errors. Now
returns 507 (Insufficient Storage) for FULL, 404 for NOT_FOUND, and
503 (Service Unavailable) when the operation queue is full.
5. TCP connection leak (root cause of proxy hang): When a peer closes a
connection, process_conn_events closes the socket and sets HUP, but
tcp_conn_free_maybe immediately frees the TCP_Conn before
tcp_next_event can deliver the HUP event. The HTTP server never learns
about the disconnect, so HTTP_Conn entries accumulate until max_conns
is reached and the proxy stops accepting new connections. Fixed by
only calling tcp_conn_free_maybe when the CLOSED flag is already set
(meaning the user called tcp_close), preserving the connection for
HUP event delivery otherwise.
Also adds wrk-based stress test suite (stress-test.sh + lua scripts).
https://claude.ai/code/session_01EHoWWnVgyCFxah5WaNV4kS
This commit is contained in:
@@ -39,7 +39,14 @@ typedef struct {
|
||||
// ID of the general state this structure is in
|
||||
TCP_ConnState state;
|
||||
|
||||
// Information about the socket
|
||||
// Information about the socket:
|
||||
// - TCP_CONN_FLAG_CLOSED
|
||||
// Whether the user is holding a handle to this struct.
|
||||
// It's first set when the TCP_EVENT_NEW is passed to
|
||||
// the user, and it's unset when the user calls tcp_close.
|
||||
// - TCP_CONN_FLAG_SECURE
|
||||
// Whether the connection was establushed via the
|
||||
// encrypted interface or not
|
||||
int flags;
|
||||
|
||||
// Events associated to this connection that the user
|
||||
@@ -57,11 +64,6 @@ typedef struct {
|
||||
// Underlying socket
|
||||
SOCKET fd;
|
||||
|
||||
// Whether the user is holding a handle to this struct.
|
||||
// It's first set when the TCP_EVENT_NEW is passed to
|
||||
// the user, and it's unset when the user calls tcp_close.
|
||||
bool handled;
|
||||
|
||||
// The socket should be closing as soon as the buffered
|
||||
// output data has been flushed. When this is set, no more
|
||||
// data can be buffered from the network.
|
||||
@@ -347,7 +349,6 @@ static void tcp_conn_init(TCP *tcp, TCP_Conn *conn, bool secure, TCP_ConnState s
|
||||
conn->state = state;
|
||||
conn->flags = 0;
|
||||
conn->events = 0;
|
||||
conn->handled = false;
|
||||
conn->closing = false;
|
||||
conn->fd = fd;
|
||||
conn->num_addrs = 0;
|
||||
@@ -506,7 +507,7 @@ static bool tcp_conn_is_buffering(TCP_Conn *conn)
|
||||
|
||||
static bool tcp_conn_free_maybe(TCP_Conn *conn)
|
||||
{
|
||||
if (!conn->handled && conn->fd == INVALID_SOCKET) {
|
||||
if (!(conn->flags & TCP_CONN_FLAG_CLOSED) && conn->fd == INVALID_SOCKET) {
|
||||
tcp_conn_free(conn);
|
||||
return true;
|
||||
} else {
|
||||
@@ -1064,6 +1065,9 @@ void tcp_write(TCP_Handle handle, string data)
|
||||
byte_queue_write_setmincap(&conn->output, data.len);
|
||||
string buf = tcp_write_buf(handle);
|
||||
|
||||
if (buf.len == 0)
|
||||
break; // Output buffer full or in error state
|
||||
|
||||
int num = MIN(buf.len, data.len);
|
||||
memcpy(buf.ptr, data.ptr, num);
|
||||
|
||||
@@ -1101,8 +1105,10 @@ void tcp_close(TCP_Handle handle)
|
||||
if (conn == NULL)
|
||||
return;
|
||||
|
||||
// Only free immediately if the user already called tcp_close
|
||||
// (CLOSED flag set). Otherwise, keep the connection alive so
|
||||
// tcp_next_event can deliver the HUP event to the user.
|
||||
conn->flags |= TCP_CONN_FLAG_CLOSED;
|
||||
conn->handled = false;
|
||||
tcp_conn_invalidate_handles(conn);
|
||||
if (tcp_conn_free_maybe(conn)) {
|
||||
tcp->num_conns--;
|
||||
|
||||
Reference in New Issue
Block a user