Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
104 changes: 90 additions & 14 deletions packages/angular/ssr/node/src/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,24 @@
import type { ServerResponse } from 'node:http';
import type { Http2ServerResponse } from 'node:http2';

/**
* Checks whether a Node.js `ServerResponse` or `Http2ServerResponse` is destroyed, closed, or ended.
*
* @param destination - The HTTP/1.1 or HTTP/2 server response to check.
* @returns `true` if the response or its underlying stream is destroyed, closed, or ended; otherwise `false`.
*/
function isResponseDestroyedOrClosed(destination: ServerResponse | Http2ServerResponse): boolean {
return (
Boolean(destination.destroyed) ||
Boolean(destination.closed) ||
Boolean(destination.writableEnded) ||
('stream' in destination &&
(!destination.stream ||
Boolean(destination.stream.destroyed) ||
Boolean(destination.stream.closed)))
);
}

/**
* Streams a web-standard `Response` into a Node.js `ServerResponse`
* or `Http2ServerResponse`.
Expand All @@ -24,6 +42,10 @@ export async function writeResponseToNodeResponse(
source: Response,
destination: ServerResponse | Http2ServerResponse,
): Promise<void> {
if (isResponseDestroyedOrClosed(destination)) {
return;
}

const { status, headers, body } = source;
destination.statusCode = status;

Expand All @@ -48,27 +70,52 @@ export async function writeResponseToNodeResponse(
}

if (!body) {
destination.end();
if (!isResponseDestroyedOrClosed(destination)) {
destination.end();
}

return;
}

try {
const reader = body.getReader();

destination.on('close', () => {
reader.cancel().catch((error) => {
// eslint-disable-next-line no-console
console.error(
`An error occurred while writing the response body for: ${destination.req.url}.`,
error,
);
});
let isClosed = isResponseDestroyedOrClosed(destination);
const isDestroyedOrClosed = () => isClosed || isResponseDestroyedOrClosed(destination);

let readerCancelled = false;
const reader = body.getReader();
const cancelReader = (error?: unknown) => {
if (readerCancelled) {
return;
}
readerCancelled = true;
isClosed = true;
destination.off('close', cancelReader);
destination.off('error', cancelReader);
reader.cancel(error).catch((err) => {
// eslint-disable-next-line no-console
console.error(
`An error occurred while writing the response body for: ${destination.req.url}.`,
Comment thread
alan-agius4 marked this conversation as resolved.
err,
);
});
};

destination.once('close', cancelReader);
destination.once('error', cancelReader);
Comment on lines +102 to +103

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is there a risk of leaking memory if ex. close is emitted, but then the error handler is still listening? Do we need to unsubscribe the other listener? I always get a little fuzzy with how event handlers behavior for GC. Looks like we're doing that below at least?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! In Node.js EventEmitter, leaving the 'error' listener attached after 'close' emits would keep cancelReader in destination._events['error'] until the finally block runs at the end of the response stream.

To make cleanup immediate and deterministic (matching the pattern used below in the backpressure Promise), I've updated cancelReader() to immediately unsubscribe both 'close' and 'error' listeners (destination.off('close', cancelReader) and destination.off('error', cancelReader)) as soon as it is invoked.


try {
// eslint-disable-next-line no-constant-condition
while (true) {
if (isDestroyedOrClosed()) {
cancelReader();
break;
}

const { done, value } = await reader.read();
if (isDestroyedOrClosed()) {
cancelReader();
break;
}

if (done) {
destination.end();
break;
Expand All @@ -78,10 +125,39 @@ export async function writeResponseToNodeResponse(
if (canContinue === false) {
// Explicitly check for `false`, as AWS may return `undefined` even though this is not valid.
// See: https://github.com/CodeGenieApp/serverless-express/issues/683
await new Promise<void>((resolve) => destination.once('drain', resolve));
await new Promise<void>((resolve) => {
if (isDestroyedOrClosed()) {
resolve();

return;
}

const onDrain = () => {
destination.off('close', onClose);
destination.off('error', onClose);
resolve();
};

const onClose = () => {
destination.off('drain', onDrain);
destination.off('close', onClose);
destination.off('error', onClose);
cancelReader();
resolve();
};
Comment thread
alan-agius4 marked this conversation as resolved.

destination.once('drain', onDrain);
destination.once('close', onClose);
destination.once('error', onClose);
});
}
}
} catch {
destination.end('Internal server error.');
if (!isDestroyedOrClosed()) {
destination.end('Internal server error.');
}
} finally {
destination.off('close', cancelReader);
destination.off('error', cancelReader);
}
}
44 changes: 44 additions & 0 deletions packages/angular/ssr/node/test/response_http1_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,48 @@ describe('writeResponseToNodeResponse (HTTP/1.1)', () => {

expect(response.headers['set-cookie']).toEqual(cookieValue);
});

it('should resolve and cancel reader when client disconnects while response is backpressured', async () => {
let writePromise!: Promise<void>;
let readerCancelled = false;
const largeChunk = 'x'.repeat(1024 * 1024 * 4); // 4MB to ensure backpressure
const stream = new ReadableStream({
start(controller) {
controller.enqueue(largeChunk);
controller.enqueue(largeChunk);
},
cancel() {
readerCancelled = true;
},
});

server.once('request', (_, nodeResponse) => {
writePromise = writeResponseToNodeResponse(new Response(stream), nodeResponse);
});

await new Promise<void>((resolve) => {
const { port } = server.address() as AddressInfo;
const clientRequest = requestCb(
{
host: 'localhost',
port,
},
(response) => {
response.once('data', () => {
clientRequest.destroy();
resolve();
});
},
);

clientRequest.on('error', () => {
// Expected when destroying the socket
});

clientRequest.end();
});

await writePromise;
expect(readerCancelled).toBeTrue();
});
});
44 changes: 44 additions & 0 deletions packages/angular/ssr/node/test/response_http2_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,48 @@ describe('writeResponseToNodeResponse (HTTP/2)', () => {

expect(resHeaders['set-cookie']).toEqual(cookieValue);
});

it('should resolve and cancel reader when client disconnects while response is backpressured', async () => {
let writePromise!: Promise<void>;
let readerCancelled = false;
const largeChunk = 'x'.repeat(1024 * 1024); // 1MB to exceed HTTP/2 flow control window
const stream = new ReadableStream({
start(controller) {
controller.enqueue(largeChunk);
controller.enqueue(largeChunk);
},
cancel() {
readerCancelled = true;
},
});

server.once('request', (_, nodeResponse) => {
writePromise = writeResponseToNodeResponse(new Response(stream), nodeResponse);
});

await new Promise<void>((resolve) => {
const { port } = server.address() as AddressInfo;
const client = connect(`http://localhost:${port}`);
const req = client.request({
':path': '/',
});

req.once('response', () => {
req.once('data', () => {
req.destroy();
client.destroy();
resolve();
});
});

req.on('error', () => {
// Expected when destroying/closing the stream
});

req.end();
});

await writePromise;
expect(readerCancelled).toBeTrue();
});
});