Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
12 changes: 9 additions & 3 deletions src/js/node/_http_incoming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,15 @@ function onDataIncomingMessage(
// Incoming request-body bytes are socket activity: push the connection's
// inactivity timeout (socket.setTimeout / server.timeout) further out, like
// Node.js does for reads on the socket.
this.socket?._unrefTimer?.();
const socket = this.socket;
socket?._unrefTimer?.();

if (chunk && !this._dumped) this.push(chunk);
if (chunk && !this._dumped) {
// Like Node's parserOnBody: pause the connection once the buffer fills.
// Skipped for upgrade (the listener owns socket flow) and once writable
// has ended (socketHandle.end() resumed the poll so kqueue sees EV_EOF).
if (!this.push(chunk) && !this.upgrade && !socket?.writableEnded) readStop(socket);
Comment thread
robobun marked this conversation as resolved.
Resolved
}

if (isLast) {
emitEOFIncomingMessage(this);
Comment thread
robobun marked this conversation as resolved.
Resolved
Expand Down Expand Up @@ -800,4 +806,4 @@ function onError(self, error, cb) {
}
}

export { IncomingMessage, kReqShouldKeepAlive, readStart, readStop };
export { IncomingMessage, kReqShouldKeepAlive, onDataIncomingMessage, readStart, readStop };
9 changes: 6 additions & 3 deletions src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const NumberIsNaN = Number.isNaN;

const { format } = require("internal/util/inspect");

const { IncomingMessage, kReqShouldKeepAlive } = require("node:_http_incoming");
const { IncomingMessage, kReqShouldKeepAlive, onDataIncomingMessage } = require("node:_http_incoming");
const {
OutgoingMessage,
kErrored,
Expand Down Expand Up @@ -865,9 +865,12 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort

setIsNextIncomingMessageHTTPS(prevIsNextIncomingMessageHTTPS);
handle.onabort = socket[kBoundOnAbort] ??= onServerRequestEvent.bind(socket);
// start buffering data if any, the user will need to resume() or .on("data") to read it
// Like Node's connectionListener -> parserOnBody: body bytes flow into
// the IncomingMessage as they arrive, and the push callback readStop()s
// the socket (which emits 'pause' on it) once the buffer fills.
if (hasBody) {
handle.pause();
handle.ondata = onDataIncomingMessage.bind(http_req);
handle.hasCustomOnData = false;
}
drainMicrotasks();

Expand Down
12 changes: 10 additions & 2 deletions src/runtime/server/NodeHTTPResponse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1354,8 +1354,13 @@
{
return Ok(JSValue::FALSE);
}
self.update_flags(|f| f.insert(Flags::IS_DATA_BUFFERED_DURING_PAUSE));
raw.on_data(on_buffer_paused_shim, self.as_ctx_ptr());
// Body already delivered: nothing to buffer, and re-arming onData would
// overwrite a pipelined request's userData on the shared HttpResponseData.
// pause_socket() still runs so pausePipelineReads can gate the fd.
if self.body_read_state.get() == BodyReadState::Pending {
self.update_flags(|f| f.insert(Flags::IS_DATA_BUFFERED_DURING_PAUSE));
raw.on_data(on_buffer_paused_shim, self.as_ctx_ptr());
}

Check warning on line 1363 in src/runtime/server/NodeHTTPResponse.rs

View check run for this annotation

Claude / Claude Code Review

body-delivered guard in do_pause/do_resume misses IS_DATA_BUFFERED_DURING_PAUSE_LAST

The new "body already delivered" guards at `do_pause` :1360 and `do_resume` :1426 test only `body_read_state != Pending`, but miss the state where `body_read_state == Pending` **and** `IS_DATA_BUFFERED_DURING_PAUSE_LAST` is set — which `on_buffer_request_body_while_paused` (:1599-1601) leaves behind when the last chunk arrives while paused. Both `set_on_data` (:2231) and `get_has_body` (:778-784) already fold this flag into their body-done predicate, and the comment at :2269-2270 documents it as
Comment thread
robobun marked this conversation as resolved.
Resolved

// TODO: figure out why windows is not emitting EOF with UV_DISCONNECT
#[cfg(not(windows))]
Expand Down Expand Up @@ -1416,6 +1421,9 @@
|| flags.contains(Flags::SOCKET_CLOSED)
|| flags.contains(Flags::ENDED)
|| flags.contains(Flags::UPGRADED)
// Body already delivered: re-arming onData/onTimeout would overwrite
// a pipelined request's userData on the shared HttpResponseData.
|| self.body_read_state.get() != BodyReadState::Pending
Comment thread
robobun marked this conversation as resolved.
Resolved
{
return JSValue::FALSE;
}
Expand Down
75 changes: 75 additions & 0 deletions test/js/node/http/node-http-req-socket-pause.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* All tests in this file should also run in Node.js.
*/
import { expect, it } from "bun:test";
import { once } from "node:events";
import { createServer, request } from "node:http";
import type { AddressInfo } from "node:net";

it("req.socket emits 'pause' once an unread request body fills the IncomingMessage buffer", async () => {
// Node's test-http-no-read-no-dump: a handler that never reads the body sees
// 'pause' on req.connection once the IncomingMessage push() backpressures.
const { promise: paused, resolve: onPause, reject } = Promise.withResolvers<number>();
const server = createServer((req, res) => {
req.connection!.on("pause", () => {
onPause((req as any).readableLength);
res.end("ok");
});
res.writeHead(200);
res.flushHeaders();
});
try {
await once(server.listen(0), "listening");
const port = (server.address() as AddressInfo).port;
const post = request({ method: "POST", port });
post.on("error", reject);
post.flushHeaders();
// One body chunk at the default highWaterMark: the first parserOnBody push
// returns false and Node's readStop pauses the socket.
post.write(Buffer.alloc(64 * 1024, "X"));
const buffered = await paused;
expect(buffered).toBeGreaterThan(0);
post.destroy();
} finally {
server.closeAllConnections();
server.close();
}
});

it("body reading from 'pause' still delivers every byte and 'end'", async () => {
// A handler that keys its slow-reader flow off the socket's 'pause' event
// (the pattern from Node's test-http-no-read-no-dump) must still be able to
// drain the full body once it attaches a 'data' listener.
const { promise: done, resolve, reject } = Promise.withResolvers<{ pauses: number; received: number }>();
const server = createServer((req, res) => {
let pauses = 0;
let received = 0;
req.connection!.on("pause", () => {
pauses++;
if (pauses > 1) return;
req.on("data", chunk => (received += chunk.length));
req.on("end", () => {
res.end("ok");
resolve({ pauses, received });
});
});
res.writeHead(200);
res.flushHeaders();
});
try {
await once(server.listen(0), "listening");
const port = (server.address() as AddressInfo).port;
const payload = 256 * 1024;
const post = request({ method: "POST", port });
post.on("error", reject);
post.flushHeaders();
await once(post, "response");
post.end(Buffer.alloc(payload, "X"));
const { pauses, received } = await done;
expect(received).toBe(payload);
expect(pauses).toBeGreaterThanOrEqual(1);
} finally {
server.closeAllConnections();
server.close();
}
});
56 changes: 56 additions & 0 deletions test/js/node/test/parallel/test-http-no-read-no-dump.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';
const common = require('../common');
const http = require('http');

let onPause = null;

const server = http.createServer(common.mustCallAtLeast((req, res) => {
if (req.method === 'GET')
return res.end();

res.writeHead(200);
res.flushHeaders();

req.on('close', common.mustCall(() => {
req.on('end', common.mustNotCall());
}));

req.connection.on('pause', () => {
res.end();
onPause();
});
})).listen(0, common.mustCall(() => {
const agent = new http.Agent({
maxSockets: 1,
keepAlive: true
});

const port = server.address().port;

const post = http.request({
agent,
method: 'POST',
port,
}, common.mustCall((res) => {
res.resume();

post.write(Buffer.alloc(64 * 1024).fill('X'));
onPause = () => {
post.end('something');
};
}));

// What happens here is that the server `end`s the response before we send
// `something`, and the client thought that this is a green light for sending
// next GET request
post.write('initial');

http.request({
agent,
method: 'GET',
port,
}, common.mustCall((res) => {
server.close();
res.connection.end();
})).end();
}));
Loading