Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: pipeline should use req.abort() to destroy response #31054

Closed
wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 21, 2019

destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change applied in 6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

This is a bugfix and I believe it should be semver minor.

Fixes #31029.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag
Copy link
Member Author

ronag commented Dec 21, 2019

Credit to @szmarczak #31029 (comment)

@szmarczak
Copy link
Member

szmarczak commented Dec 21, 2019

I have been just running git clone ... right now :P The speed is about 500KB/s (speedtest results show 2.5MB/s) - that's quite slow. I don't know if this is because of my location or something else... Thanks for taking care of this :)

Edit 1: I guess it's just my location, on my VPS I get 10MB/s.
Edit 2: I set up a forward proxy, it's running at full speed now :D

test/parallel/test-stream-pipeline.js Outdated Show resolved Hide resolved
test/parallel/test-stream-pipeline.js Outdated Show resolved Hide resolved
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change introduced in nodejs@6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: nodejs#31029
Refs: nodejs@6480882
@ronag
Copy link
Member Author

ronag commented Dec 21, 2019

@mcollina: You ok with this instead of reverting 6480882?

test/parallel/test-stream-pipeline.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@lpinca lpinca added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 22, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2019
res,
stream,
common.mustCall((err) => {
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to also validate the error here?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. At least that err is truthy.

Copy link
Member Author

@ronag ronag Dec 25, 2019

Choose a reason for hiding this comment

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

Fixed. Unfortunately we get an unexpected/different error (ERR_STREAM_DESTROYED) due to the behavior discussed in #31060. So I made it just check for truthyness.

Copy link
Member

@lpinca lpinca Dec 25, 2019

Choose a reason for hiding this comment

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

I can't understand how this is related to #31060. AFAIK none of the _destroy() implementations is async.

It seems a regression on master to me

const stream = require('stream');

const data = Buffer.alloc(1024);

{
  const readable = new stream.Readable({
    read() {
      this.push(data);
    }
  });

  const writable = new stream.Writable({
    write(chunk, encoding, callback) {
      callback();
    }
  });

  writable.on('error', console.error);

  readable.pipe(writable);
  writable.destroy(new Error('Oops'));
}

{
  const readable = new stream.Readable({
    read() {
      this.push(data);
    }
  });

  const writable = new stream.Writable({
    write(chunk, encoding, callback) {
      callback();
    }
  });

  stream.pipeline(readable, writable, console.error);

  writable.destroy(new Error('Oops'));
}

This prints

Error: Oops
    at Object.<anonymous> (/Users/luigi/Desktop/pipe.js:23:20)
    at Module._compile (internal/modules/cjs/loader.js:1139:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1159:10)
    at Module.load (internal/modules/cjs/loader.js:988:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47
Error: Oops
    at Object.<anonymous> (/Users/luigi/Desktop/pipe.js:41:20)
    at Module._compile (internal/modules/cjs/loader.js:1139:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1159:10)
    at Module.load (internal/modules/cjs/loader.js:988:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

on Node.js 13.5.0 and

Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed
    at Writable.write (_stream_writable.js:321:17)
    at Readable.ondata (_stream_readable.js:779:22)
    at Readable.emit (events.js:320:20)
    at Readable.read (_stream_readable.js:579:10)
    at flow (_stream_readable.js:1052:34)
    at resume_ (_stream_readable.js:1033:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  code: 'ERR_STREAM_DESTROYED'
}
Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed
    at Writable.write (_stream_writable.js:321:17)
    at Readable.ondata (_stream_readable.js:779:22)
    at Readable.emit (events.js:320:20)
    at Readable.read (_stream_readable.js:579:10)
    at flow (_stream_readable.js:1052:34)
    at resume_ (_stream_readable.js:1033:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  code: 'ERR_STREAM_DESTROYED'
}

on Node.js master

Copy link
Member

Choose a reason for hiding this comment

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

Bisecting points to 67ed526.

Copy link
Member

@lpinca lpinca Dec 25, 2019

Choose a reason for hiding this comment

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

If that _destroy() completed with an error yes, totally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. I’ll add it to the proposals list.

Copy link
Member

Choose a reason for hiding this comment

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

How can this thread be resolved? We need a fix with the bug this PR aims to solve for node v13, or better revert the few commits that caused the problem in the first place.

Copy link
Member Author

@ronag ronag Dec 25, 2019

Choose a reason for hiding this comment

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

I don't think this specific thread needs to be urgently resolved (or at the least it's a different issue). This PR in its current form resolves the original issue.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this discussion should not block the PR. The issue discussed here is caused by a semver-major change that will not be included in v13.x

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Dec 25, 2019

Landed in c852f7e

Trott pushed a commit that referenced this pull request Dec 25, 2019
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change introduced in 6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: #31029
Refs: 6480882

PR-URL: #31054
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott Trott closed this Dec 25, 2019
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change introduced in 6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: #31029
Refs: 6480882

PR-URL: #31054
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@targos
Copy link
Member

targos commented Jan 14, 2020

This lands without issues on v12.x-staging after #30869 but the new test fails because the 'error' event is emitted twice.

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change introduced in nodejs@6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: nodejs#31029
Refs: nodejs@6480882

PR-URL: nodejs#31054
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
destroy(err) on http response will propagate the error to the
request causing 'error' to be unexpectedly emitted. Furthermore,
response.destroy() unlike request.abort() does not _dump buffered
data.

Fixes a breaking change introduced in 6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: #31029
Refs: 6480882

PR-URL: #31054
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stream.pipeline(httpResponse, decompressStream) emits error three times
9 participants