bun
bun copied to clipboard
oven-sh
IncomingRequest fails to emit `close` event when client disconnects
What version of Bun is running?
1.1.7+b0b7db5c0
What platform is your computer?
Darwin 23.2.0 arm64 arm
What steps can reproduce the bug?
Creating a Server and attempting to use the req.on('close') to manage cleanup for for when a request closes does not work when using the Bun runtime.
The example below simply requires running the example.js file using bun example.js and navigating to http://localhost:3000, the page will load, connect to the /event route and begin handling Server Sent Events. After 3 server sent events the client will close the connection, but the close event is never emitted. This is true for all requests, not just in the Sever Sent Events use case.
Navigating to the /slow-response route, which sets a timeout for 10 seconds before responding to the client, and cancelling the request before it fulfills also produces the same results. The close event is not fired, which doesn't allow the cleanup to prevent the timeout (or interval in the SSE use case), of course leading to memory leaks and undeterminable connection state.
// example.js
import { createServer } from 'node:http';
const BASIC_HTML = `<html>
<head>
<title>Server Sent Events</title>
</head>
<body>
<h1>Server Sent Events</h1>
<div id="events"></div>
<script>
function addEvent(data) {
const events = document.getElementById('events');
const newItem = document.createElement('div');
newItem.innerHTML = new Date() + ' ' + JSON.stringify(data);
events.appendChild(newItem);
}
let eventCount = 0;
const eventSource = new EventSource('/events');
function closeConnection() {
eventSource.close();
addEvent({ type: 'closing_connection' });
}
eventSource.onerror = function(err) {
addEvent({ type: 'error', error: err });
closeConnection();
}
eventSource.addEventListener('action', function(event) {
addEvent({ type: 'received_action', data: event.data});
eventCount++;
if (eventCount >= 3) {
closeConnection();
}
});
window.onbeforeunload = closeConnection;
</script>
</body>
`;
function sendHomePage(req, res) {
console.log('HomeRoute')
res.writeHead(200, { 'Content-Type': 'text/html' });
res.end(BASIC_HTML);
}
function handleSlowRoute(req, res) {
console.log('SlowRoute')
const timeout = setTimeout(() => {
console.log('sending response from inside timeout');
res.writeHead(200, { 'Content-Type': 'text/html' });
res.end('hello world');
}, 10000);
req.on('close', () => {
console.log('closing connection');
clearTimeout(timeout);
});
}
function handleEvents(req, res) {
console.log('EventsRoute')
res.writeHead(200, {
'Content-Type': 'text/event-stream',
'Cache-Control': 'no-cache',
'Connection': 'keep-alive'
});
console.log('sending connection event')
res.write(`event: connection\ndata: connected\n\n`);
let eventCount = 0;
const interval = setInterval(() => {
console.log('sending event')
res.write(`event: action\ndata: ${JSON.stringify({ type: 'test', count: ++eventCount })}\n\n`);
}, 2000);
req.on('close', () => {
console.log('closing connection');
clearInterval(interval);
});
}
const server = createServer((req, res) => {
switch (req.url) {
case '/':
return sendHomePage(req, res);
case '/events':
return handleEvents(req, res);
case '/slow-route':
return handleSlowRoute(req, res);
default:
res.writeHead(404);
res.end();
}
});
server.listen(3000, () => {
console.log('Server is running on port 3000');
});
What is the expected behavior?
λ node example.js
Server is running on port 3000
SlowRoute
closing connection # <-- Good. caused from navigating to /slow-route and cancelling the request before it fulfilled.
HomeRoute
EventsRoute
sending connection event
sending event
sending event
sending event
closing connection # <-- Good. caused from calling `eventSource.close()` from the browser.
What do you see instead?
λ bun example.js
Server is running on port 3000
SlowRoute
sending response from inside timeout # <-- Bad. this should not have fired. should have received a "closing connection" log and not sent a response after the client cancelled the request.
HomeRoute
EventsRoute
sending connection event
sending event
sending event
sending event
sending event # <-- Bad. from here down should not have happened. should have received a "closing connection" log.
sending event
sending event
sending event
sending event
sending event
sending event
sending event
^C # <-- terminated the process.
Additional information
Nothing more related to this issue, but wanted to commend @Jarred-Sumner and the other Bun team members for their work on this project. So far, i love Bun and hope to see it go the distance! 🥇
Tests
Below are two tests in bun. the first one passes as it emits the close event when the req.destroy() method is called from the server. The second test fails as the close event is not emitted when the client aborts/disconnects the request.
Additionally, there's a Node.js Equivalent test showing the expected behavior and results. It's slightly different as the node testing framework doesn't support both a done callback with async/await. So, the createAsyncTestResolver allows for that behavior to be mimicked.
Bun Tests
-
test/js/node/http/node-http.test.ts
it("should emit `close` event on IncomingRequest when request is destroyed", async done => {
try {
const SERVER_RESPONSE = "event_emitted";
var server = createServer((req, res) => {
req.on("close", () => {
res.end(SERVER_RESPONSE);
done();
});
res.writeHead(200, { "Content-Type": "text/html" });
req.destroy();
});
const url = new URL("/event-based-response-endpoint", await listen(server));
const response_text = await fetch(url).then(res => res.text());
expect(response_text).toBe(SERVER_RESPONSE);
} catch (e) {
throw e;
} finally {
server.close();
}
});
it("should emit `close` event on IncomingRequest when client disconnects", async done => {
try {
const response_interval_duration_ms = 100;
const client_request_abort_duration_ms = 50;
var server = createServer((req, res) => {
res.writeHead(200, { "Content-Type": "text/html" });
let responseInterval = setInterval(() => {
clearInterval(responseInterval); // cleanup: remove testing interval as close event failed to emit and cannot be cleaned up.
res.write(`content sent at ${new Date().toISOString()}\n`);
done(new Error("MemoryLeak -- failed to emit `close` event on client disconnection"));
}, response_interval_duration_ms);
req.on("close", () => {
clearInterval(responseInterval);
done(); // allow test to pass if client disconnection is handled by the IncomingRequest `close` event.
});
});
const url = new URL("/event-based-response-endpoint", await listen(server));
const controller = new AbortController();
setTimeout(() => controller.abort(), client_request_abort_duration_ms);
const abortedRequestErrorMessage = await fetch(url, { signal: controller.signal }).catch(e => e.name);
expect(abortedRequestErrorMessage).toBe("AbortError");
} catch (e) {
throw e;
} finally {
server.close();
}
});
Output:
✓ node:http > request > should emit `close` event on IncomingRequest when request is destroyed [3.22ms]
854 | var server = createServer((req, res) => {
855 | res.writeHead(200, { "Content-Type": "text/html" });
856 | let responseInterval = setInterval(() => {
857 | clearInterval(responseInterval); // cleanup: remove testing interval as close event failed to emit and cannot be cleaned up.
858 | res.write(`content sent at ${new Date().toISOString()}\n`);
859 | throw new Error("MemoryLeak -- failed to emit `close` event on client disconnection");
^
error: MemoryLeak -- failed to emit `close` event on client disconnection
at /Users/david/Sites/bun/test/js/node/http/node-http.test.ts:859:19
✗ node:http > request > should emit `close` event on IncomingRequest when client disconnects [109.20ms]
Node.js Equivalent Tests
import { describe, it } from 'node:test';
import assert from 'node:assert';
import { createServer } from 'node:http';
function expect(value) {
return {
toBe: function (expected) {
assert.equal(value, expected);
}
}
}
function createAsyncTestResolver() {
let resolve;
let asyncTestResolver = new Promise(_resolve => resolve = _resolve);
asyncTestResolver.done = resolve;
return asyncTestResolver
}
describe('node:http', async () => {
describe('IncomingRequest', async () => {
it("emits `close` event when client disconnects", async () => {
try {
const asyncTestResolver = createAsyncTestResolver();
var server = createServer((req, res) => {
res.writeHead(200, { 'Content-Type': 'text/html' });
const responseInterval = setInterval(() => {
clearInterval(responseInterval);
res.write('hello world');
throw new Error('timeout not cleaned up -- memmory leak')
}, 100);
req.on('close', () => {
clearInterval(responseInterval);
asyncTestResolver.done();
});
});
await new Promise(resolve => server.listen(3000, resolve));
const controller = new AbortController();
setTimeout(() => controller.abort(), 50);
const res = await fetch("http://localhost:3000", { signal: controller.signal }).catch(e => e.name);
expect(res).toBe("AbortError");
await asyncTestResolver;
} catch (e) {
throw e;
} finally {
server.close();
}
});
});
});
Output:
λ node tests.js
▶ node:http
▶ IncomingRequest
✔ emits `close` event when client disconnects (56.966875ms)
▶ IncomingRequest (58.043ms)
▶ node:http (58.506584ms)
ℹ tests 1
ℹ suites 2
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 63.143417
Ran into this myself, apparently no way around it - actually, none of these events seem to fire: close, connect, data, drain, end, error, lookup, ready, timeout
@Electroid any way this can get re-reviewed and prioritized? It seems this needs the node.js label, will be the culprit of significant memory leaks for many applications, and breaks the ideal of a node drop-in replacement.
I just ran into this on (v1.1.18) :( I was hoping maybe as a hacky workaround I could poll for socket.closed but that also doesn't seem to ever be set to true.
I was looking at src/js/node/http.ts and trying to think of anything obvious, but wondering if it in the other ts or zig side (which not experienced with). Wishing I knew how I could help patch and fix this. I guess for now, just work on other parts of my project than worrying about graceful shutdown logic and comment it out for now.
