fix(storage): handle stream teardown before pipeline setup#8669
fix(storage): handle stream teardown before pipeline setup#8669rockwotj wants to merge 2 commits into
Conversation
product-auto-label
Bot
added
the
api: storage
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stream cleanup improvements in File operations, including helper functions to clean up requests and raw response streams, handling early destruction of streams in createReadStream, and adding error handling during write stream initialization. A test case is also added to verify proper cleanup. One issue was identified in createWriteStream where the temporary onPrePipelineError listener on fileWriteStream is not removed if an early return occurs due to a destroyed stream, which could lead to memory leaks or unexpected callbacks.
rockwotj
force-pushed
the
fix-storage-create-read-stream-pipe-race
branch
from
60478b6 to
0192df3
Compare
File#createReadStream emits the response event before attaching the raw response pipeline. A consumer can destroy the returned stream from that event, leaving pipeline() to receive a destroyed destination and throw ERR_STREAM_UNABLE_TO_PIPE synchronously. Check the public stream after the response arrives and discard the raw response body when the consumer has already closed it. Reuse the existing request-agent cleanup so the abandoned response does not retain its underlying socket resources. Add a regression test using the real stream event ordering. The test destroys the returned stream from its response handler and verifies that the delayed raw response and request agent are cleaned up. This adapts the core approach from PR googleapis#7604 by Michael Latman, which was closed before merge.
createWriteStream starts the simple or resumable upload before installing the pipeline that normally owns fileWriteStream errors. If startup fails synchronously, fileWriteStream and the public write stream can be destroyed before pipeline() runs. Node then throws ERR_STREAM_UNABLE_TO_PIPE while attaching invalid streams, and the original upload error can escape uncaught. Install a temporary error listener before upload startup and forward any early failure through the existing pipeline callback. If startup has already torn down any stream, skip pipeline setup and destroy the remaining internal streams. Keep the temporary listener until the upload stream closes so a queued terminal error is still handled, then remove it to avoid retaining the callback. Add coverage for a consumer destroying the returned stream during upload startup. The test verifies that the internal upload stream is destroyed and its temporary error listener is removed. Existing synchronous upload-error and pipeline-failure coverage continues to pass.
rockwotj
force-pushed
the
fix-storage-create-read-stream-pipe-race
branch
from
0192df3 to
fd9edf1
Compare
1 participant
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #7603
Summary
File#createReadStream()emits the response event before attaching its raw-response pipeline. A consumer can destroy the returned stream from that event, causingpipeline()to receive a destroyed destination and throwERR_STREAM_UNABLE_TO_PIPEsynchronously.This PR checks the public stream after the response arrives. If it has already been destroyed, the raw response and request agent are cleaned up instead of constructing the pipeline.
The branch also contains a separate commit for the analogous
createWriteStream()startup failure currently exposed by newer Node releases. Upload startup can emit an error and destroy the streams before the normal pipeline error handler is installed. A temporary listener preserves that original error until teardown completes. Pipeline setup is skipped when startup has already torn down either side, and any remaining internal upload stream is explicitly destroyed.Attribution
The read-stream fix adapts the core destroyed-stream guard from #7604 by @michaellatman, which was closed before merge. This version uses the real stream event ordering in the regression test and does not retain the additional synchronous
pipeline()try/catch.Testing
ERR_STREAM_UNABLE_TO_PIPEbefore the source fix and passes afterward.Credentialed Cloud Storage system tests were not run locally.