fix(@angular/ssr): settle writeResponseToNodeResponse when client disconnects - #33721
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
alan-agius4
added
the
target: patch
2cd9689 to
e4a510a
Compare
alan-agius4
added
action: review
There was a problem hiding this comment.
Code Review
This pull request introduces robust handling for closed or destroyed Node.js server responses during streaming. It adds helper checks to prevent writing to terminated responses and ensures that the response stream reader is properly cancelled and cleaned up when a client disconnects or an error occurs, particularly under backpressure conditions. Tests have been added for both HTTP/1.1 and HTTP/2 to verify this behavior. The review feedback suggests using optional chaining when accessing destination.req.url to prevent potential TypeErrors in environments where req might be undefined.
| destination.once('close', cancelReader); | ||
| destination.once('error', cancelReader); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…connects When a client disconnects while a response is backpressured or streaming, the Node response (`ServerResponse` or `Http2ServerResponse`) is closed or destroyed without emitting a `drain` event. Previously, this caused `writeResponseToNodeResponse()` to park indefinitely waiting for `drain` and never settle. This change monitors whether the Node response is closed or destroyed and removes event listeners, cancels the reader, and resolves the returned Promise when a client disconnect occurs. Closes angular#33719
alan-agius4
added
action: merge
2 participants
When a client disconnects while a response is backpressured or streaming, the Node response (
ServerResponseorHttp2ServerResponse) is closed or destroyed without emitting adrainevent. Previously, this causedwriteResponseToNodeResponse()to park indefinitely waiting fordrainand never settle.This change monitors whether the Node response is closed or destroyed and removes event listeners, cancels the reader, and resolves the returned Promise when a client disconnect occurs.
Closes #33719