Skip to content

fix(teeny-request): destroy response stream if request stream is destroyed early - #9160

Draft
westarle wants to merge 1 commit into
googleapis:mainfrom
westarle:issue-8670-fix
Draft

fix(teeny-request): destroy response stream if request stream is destroyed early#9160
westarle wants to merge 1 commit into
googleapis:mainfrom
westarle:issue-8670-fix

Conversation

@westarle

Copy link
Copy Markdown
Contributor

If the consumer destroys the returned stream before piping starts, teeny-request should skip pipeline construction and destroy the unused response body. This prevents unhandled rejections (e.g. ERR_STREAM_UNABLE_TO_PIPE) when trying to pipe to a closed/destroyed stream.

Fixes: 8670

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request prevents piping streams if the request stream is destroyed early, and ensures the response stream is properly destroyed instead. It also adds a test case to verify this behavior. The review feedback suggests simplifying a redundant null-check on responseStream and refactoring the test to avoid using setTimeout, which can cause flakiness in CI/CD environments.

Comment on lines +281 to +285
if (!requestStream.destroyed) {
pipeline(responseStream, requestStream, () => {});
} else if (responseStream && typeof responseStream.destroy === 'function') {
responseStream.destroy();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The check responseStream && is redundant here because this block is already nested inside if (responseStream) on line 280. We can safely simplify this condition.

        if (!requestStream.destroyed) {
          pipeline(responseStream, requestStream, () => {});
        } else if (typeof responseStream.destroy === 'function') {
          responseStream.destroy();
        }

Comment on lines +301 to +324
it('should not pipe if request stream is destroyed early', done => {
const scope = mockJson();
let bodyDestroyed = false;

const stream = teenyRequest({uri});
stream.on('error', done);

stream.once('response', response => {
response.body.once('close', () => {
bodyDestroyed = true;
});

// Destroy the stream before piping can be fully set up
stream.destroy();

setTimeout(() => {
assert.strictEqual(bodyDestroyed, true);
scope.done();
done();
}, 50);
});

stream.resume();
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using an arbitrary setTimeout in asynchronous tests can lead to flakiness in slower CI/CD environments. Since we expect the stream to be destroyed, we can listen directly to the close event of the response body to resolve the test, which is more robust and faster.

  it('should not pipe if request stream is destroyed early', done => {
    const scope = mockJson();

    const stream = teenyRequest({uri});
    stream.on('error', done);

    stream.once('response', response => {
      response.body.once('close', () => {
        scope.done();
        done();
      });

      // Destroy the stream before piping can be fully set up
      stream.destroy();
    });

    stream.resume();
  });

…royed early

If the consumer destroys the returned stream before piping starts,
teeny-request should skip pipeline construction and destroy the unused
response body. This prevents unhandled rejections (e.g. ERR_STREAM_UNABLE_TO_PIPE)
when trying to pipe to a closed/destroyed stream.

Fixes: 8670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant