Repository navigation
fix(ext/node): stop http2 file reads after stream close - #36300
Conversation
HTTP/2 respondWithFile and respondWithFD send file data by chaining fs.read callbacks and writing each chunk to the stream. If the peer closes the connection while a file response is in progress, the stream can close while that read loop still has work scheduled. The loop should not keep reading from the file after the stream has no consumer. Stop the read loop when the stream closes. If a read is already in flight, defer closing an owned file descriptor until that read callback returns. This avoids racing fs.close() against fs.read(), while still making connection aborts stop scheduling more file reads. Keep the existing read-error path intact so actual read failures still reset and destroy the stream as before. Normal completion and response setup failures still close owned file descriptors once.
HTTP/2 file responses mark an async read as active before calling fs.read. The normal callback path clears that marker before deciding whether to close an owned file descriptor. If fs.read throws before it returns, the marker stayed set. A later stream close would then stop the read loop but skip closing the owned descriptor because it still looked like a read was in flight. Handle synchronous fs.read failures with the same cleanup path used for asynchronous read errors. Clear the active-read marker before destroying the stream, and add unit coverage that makes fs.read throw while respondWithFile owns the descriptor.
bartlomieju
changed the title
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
LGTM β the deferral logic is correct. Tracing the paths: stopReading() now defers closeOwnedFd() while a read is in flight (reading), and the fd is closed when the read callback returns and re-invokes stopReading(). Dropping the old if (stopped) return guard is safe because closeOwnedFd() self-guards with fdClosed and removeListener is idempotent, so the fd closes exactly once across the finish/error/close paths. The try/catch correctly handles a synchronous fs.read throw and routes to handleReadError(), and the error path still resets the stream with NGHTTP2_INTERNAL_ERROR as before.
One optional addition: the new test exercises the sync-throw fd-close path, but not the core race being fixed β a stream closing while a read is genuinely in flight, asserting the fd stays open until the read callback returns. The existing testRespondWithCancellation covers cancellation more broadly, so this is non-blocking.
Written by Codex; split off from #35657 by request
HTTP/2 respondWithFile and respondWithFD send file data by chaining fs.read callbacks and writing each chunk to the stream. If the peer closes the connection while a file response is in progress, the stream can close while that read loop still has work scheduled. The loop should not keep reading from the file after the stream has no consumer.
Stop the read loop when the stream closes. If a read is already in flight, defer closing an owned file descriptor until that read callback returns. This avoids racing fs.close() against fs.read(), while still making connection aborts stop scheduling more file reads.
Keep the existing read-error path intact so actual read failures still reset and destroy the stream as before. Normal completion and response setup failures still close owned file descriptors once.