Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/bun-usockets/src/crypto/openssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -2098,6 +2098,16 @@ void *us_internal_ssl_get_native_handle(struct us_socket_t *s) {
return s->ssl ? s_ssl(s) : NULL;
}

/* Ciphertext bytes already sealed for `s` and counted as written by
* us_internal_ssl_write, still waiting on a writable event to reach the
* kernel. uWS's getBufferedAmount() adds this so its close-after-drain
* gates do not fire while the last batch is still in userspace. */
unsigned int us_internal_ssl_spill_pending(struct us_socket_t *s) {
struct loop_ssl_data *loop_ssl_data = (struct loop_ssl_data *)s->group->loop->data.ssl_data;
if (!loop_ssl_data || loop_ssl_data->ssl_spill_owner != s) return 0;
return loop_ssl_data->ssl_spill_len - loop_ssl_data->ssl_spill_off;
}

int us_internal_ssl_write(struct us_socket_t *s, const char *data, int length) {
if (us_socket_is_closed(s) || us_internal_ssl_is_shut_down(s) || length == 0) return 0;

Expand Down
1 change: 1 addition & 0 deletions packages/bun-usockets/src/internal/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ int us_internal_ssl_handshake_callback_has_fired(us_socket_r s);
int us_internal_ssl_is_shut_down(us_socket_r s);
void us_internal_ssl_shutdown(us_socket_r s);
int us_internal_ssl_write(us_socket_r s, const char *data, int length);
unsigned int us_internal_ssl_spill_pending(us_socket_r s);
void *us_internal_ssl_get_native_handle(us_socket_r s);
struct us_bun_verify_error_t us_internal_ssl_verify_error(us_socket_r s);
void *us_internal_ssl_sni_userdata(us_socket_r s);
Expand Down
5 changes: 5 additions & 0 deletions packages/bun-usockets/src/libusockets.h
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,11 @@ int us_socket_is_closed(us_socket_r s) nonnull_fn_decl;
int us_socket_is_tls(us_socket_r s) nonnull_fn_decl;
int us_socket_is_ssl_handshake_finished(us_socket_r s) nonnull_fn_decl;
int us_socket_ssl_handshake_callback_has_fired(us_socket_r s) nonnull_fn_decl;
/* TLS ciphertext bytes already sealed for this socket and reported as
* written by us_socket_write(), still waiting on a writable event to reach
* the kernel (the loop-wide spill slot owned by this socket). 0 for
* plain-TCP sockets and for TLS sockets with nothing spilled. */
unsigned int us_socket_ssl_spill_pending(us_socket_r s) nonnull_fn_decl;

struct us_socket_t *us_socket_close(us_socket_r s, int code, void *reason) __attribute__((nonnull(1)));

Expand Down
7 changes: 7 additions & 0 deletions packages/bun-usockets/src/socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ int us_socket_ssl_handshake_callback_has_fired(struct us_socket_t *s) {
return 1;
}

unsigned int us_socket_ssl_spill_pending(struct us_socket_t *s) {
if (s->ssl) {
return us_internal_ssl_spill_pending(s);
}
return 0;
}

int us_connecting_socket_is_closed(struct us_connecting_socket_t *c) {
return c->closed;
}
Expand Down
12 changes: 10 additions & 2 deletions packages/bun-uws/src/AsyncSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,16 @@ struct AsyncSocket {
/* Returns the user space backpressure. */
size_t getBufferedAmount() {
/* Unsent bytes; already-written bytes sitting behind the head cursor
* are not backpressure (maxBackpressure checks, drain progress). */
return getAsyncSocketData()->buffer.length();
* are not backpressure (maxBackpressure checks, drain progress).
* For TLS, us_socket_write() can report a batch as written while its
* ciphertext still sits in the loop's spill slot (openssl.c
* ssl_flush_write_batch); count that so the close-after-drain gates
* in HttpResponse/HttpContext/WebSocketContext wait for it. */
size_t buffered = getAsyncSocketData()->buffer.length();
if constexpr (SSL) {
buffered += us_socket_ssl_spill_pending((us_socket_t *) this);
}
return buffered;
}

/* Returns the text representation of an IPv4 or IPv6 address */
Expand Down
15 changes: 7 additions & 8 deletions packages/bun-uws/src/HttpContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,13 @@ struct HttpContext {
/* A peer FIN must not tear the connection down at the loop level:
* onEnd() below decides whether to close right away (idle) or to
* keep writing the responses that are still in flight / pipelined
* (Node's socketOnEnd semantics). Without this flag the loop
* force-closes the socket right after dispatching onEnd. TLS
* (openssl.c us_internal_ssl_on_end) does not consult this flag
* and force-closes on FIN regardless, so this half of the compat
* block is http-only for now. */
if constexpr (!SSL) {
s->flags.allow_half_open = 1;
}
* (Node's socketOnEnd semantics). Without this flag the loop (and
* openssl.c us_internal_ssl_on_end for TLS) force-closes the
* socket right after dispatching onEnd, discarding the buffered
* response bytes. getBufferedAmount() counts the TLS spill, so
* onEnd's defer and onWritable's close gate are accurate for both
* transports. */
Comment thread
robobun marked this conversation as resolved.
Outdated
s->flags.allow_half_open = 1;
}

if(!SSL) {
Expand Down
79 changes: 79 additions & 0 deletions test/js/node/http/node-http-backpressure.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
* A handful of older tests do not run in Node in this file. These tests should be updated to run in Node, or deleted.
*/
import { once } from "node:events";
import { readFileSync } from "node:fs";
import http from "node:http";
import https from "node:https";
import type { AddressInfo } from "node:net";
import net from "node:net";
import path from "node:path";
import nodeTls from "node:tls";

describe("backpressure", () => {
// Writes `total` bytes to `res` in `chunk`-sized pieces, waiting for "drain"
Expand Down Expand Up @@ -197,6 +201,81 @@
// rejects the second write (socketOnEnd already called socket.end()); Bun
// currently accepts and drains it. Both are consistent: the client sees
// either the first write only, or both, never a torn second write.
// Same scenario over TLS: the server's TLS write-batch spill (up to one
// 128 KiB ciphertext batch the kernel did not fully accept) must be
// counted as buffered and drained before the post-FIN close gate fires.
// Cycling several times proves the on_writable drain loop, not just the
// first kernel-accepted write.
describe("https", () => {

Check warning on line 209 in test/js/node/http/node-http-backpressure.test.ts

View check run for this annotation

Claude / Claude Code Review

New describe("https") block detaches an existing why-comment from its test

The new `describe("https")` block was inserted between a pre-existing why-comment (lines 198–203, "A 'drain' listener that writes again … never a torn second write") and the test it documents (`it("res.write() from 'drain' after client FIN is not torn mid-write")`, now at line 279). Appending the "Same scenario over TLS" comment onto that block and putting ~70 lines of https tests after it makes the original comment read as documentation for the https block — which has no torn-second-write case
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
const keysDir = path.join(import.meta.dirname, "..", "test", "fixtures", "keys");
const tlsOptions = {
cert: readFileSync(path.join(keysDir, "agent1-cert.pem")),
key: readFileSync(path.join(keysDir, "agent1-key.pem")),
};

async function halfCloseTlsRequestBodyBytes(
port: number,
request: string,
halfClose: boolean,
): Promise<{ body: number; ended: boolean }> {
const socket = nodeTls.connect({ port, host: "127.0.0.1", rejectUnauthorized: false });
let body = 0;
let head = "";
let gotHead = false;
let ended = false;
socket.on("data", chunk => {
if (!gotHead) {
head += chunk.toString("latin1");
const i = head.indexOf("\r\n\r\n");
if (i >= 0) {
gotHead = true;
body = Buffer.byteLength(head.slice(i + 4), "latin1");
}
} else {
body += chunk.length;
}
});
socket.on("end", () => (ended = true));
socket.on("error", () => {});
await once(socket, "secureConnect");
if (halfClose) socket.end(request);
else socket.write(request);
await once(socket, "close");
return { body, ended };
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

it.each([
["client half-close, res.write() then res.end()", "write-end", true],
["client half-close, res.end(payload)", "end", true],
["client half-close, httpAllowHalfOpen, res.end() after drain", "drain", true],

Check warning on line 250 in test/js/node/http/node-http-backpressure.test.ts

View check run for this annotation

Claude / Claude Code Review

Dead halfClose parameter in https test helper

The `halfClose` parameter on `halfCloseTlsRequestBodyBytes` is dead: all three `it.each` rows pass `true`, so the `else socket.write(request)` branch is never taken. Either drop the parameter/branch (the helper's name already implies half-close) or add the intended non-half-close row to the matrix.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
] as const)("%s", async (_name, endMode, halfClose) => {
await using server = https.createServer(tlsOptions, (req, res) => {
res.writeHead(200, { "Content-Length": String(BODY) });
if (endMode === "end") {
res.end(payload);
} else {
res.write(payload);
if (endMode === "write-end") res.end();
else res.once("drain", () => res.end());
}
});
if (endMode === "drain") server.httpAllowHalfOpen = true;
await once(server.listen(0, "127.0.0.1"), "listening");
const port = (server.address() as AddressInfo).port;
// Loop a few times: the bug is an ordering race between the final
// spilled TLS batch and the close gate, so a single pass on a fast
// loopback may not catch the last-batch truncation.
for (let i = 0; i < 5; i++) {
const { body, ended } = await halfCloseTlsRequestBodyBytes(
port,
"GET / HTTP/1.1\r\nHost: localhost\r\n\r\n",
halfClose,
);
expect({ body, ended }).toEqual({ body: BODY, ended: true });
}
});
});

it("res.write() from 'drain' after client FIN is not torn mid-write", async () => {
await using server = http.createServer((req, res) => {
res.writeHead(200, { "Content-Length": String(BODY * 2) });
Expand Down
Loading