stream: pipeline should use req.abort() to destroy response by ronag · Pull Request #31054 · nodejs/node · GitHub

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

Credit to @szmarczak #31029 (comment)

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

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/27855/

lgtm

CI: https://ci.nodejs.org/job/node-test-pull-request/27882/

Does it make sense to also validate the error here?

I agree. At least that err is truthy.

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.

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

Bisecting points to 67ed526.

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

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

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.

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.

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

lgtm

Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>

CI: https://ci.nodejs.org/job/node-test-pull-request/27937/

CI: https://ci.nodejs.org/job/node-test-pull-request/27940/

CI: https://ci.nodejs.org/job/node-test-pull-request/27943/

Landed in c852f7e

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 <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

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 <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

ronag force-pushed the pipeline-response-destroy branch from b4a4544 to 16b4ed4 Compare December 21, 2019 11:14

szmarczak reviewed Dec 21, 2019

View reviewed changes

Comment thread test/parallel/test-stream-pipeline.js Outdated

lpinca reviewed Dec 21, 2019

View reviewed changes

Comment thread test/parallel/test-stream-pipeline.js Outdated

Comment thread test/parallel/test-stream-pipeline.js Outdated

ronag force-pushed the pipeline-response-destroy branch from 16b4ed4 to 6dd8500 Compare December 21, 2019 11:39

lpinca approved these changes Dec 21, 2019

View reviewed changes

Comment thread test/parallel/test-stream-pipeline.js Outdated

fixup: nit

b7a0f75

szmarczak approved these changes Dec 21, 2019

View reviewed changes

mcollina approved these changes Dec 21, 2019

View reviewed changes

lpinca added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 22, 2019

Qard approved these changes Dec 22, 2019

View reviewed changes

Trott approved these changes Dec 23, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 23, 2019

BridgeAR approved these changes Dec 24, 2019

View reviewed changes

fixup

039b944

ronag mentioned this pull request Dec 25, 2019

stream: destroy(err) predictability #31060

Closed

mcollina approved these changes Dec 25, 2019

View reviewed changes

BridgeAR reviewed Dec 25, 2019

View reviewed changes

Comment thread test/parallel/test-stream-pipeline.js Outdated

Update test/parallel/test-stream-pipeline.js

14c816d

Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott closed this Dec 25, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos added the backport-requested-v12.x label Jan 14, 2020

targos mentioned this pull request Jan 14, 2020

stream: make all streams error in a pipeline #30869

Closed

4 tasks

szmarczak mentioned this pull request Mar 5, 2020

Pipeline aborts all HTTP requests #32105

Closed

targos added backport-open-v12.x and removed backport-requested-v12.x labels Apr 25, 2020

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

+                      res,
+                      stream,
+                      common.mustCall((err) => {
+                        server.close();

stream: pipeline should use req.abort() to destroy response by ronag · Pull Request #31054 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

stream: pipeline should use req.abort() to destroy response#31054ronag wants to merge 4 commits intonodejs:masternodejs/node:masterfrom nxtedition:pipeline-response-destroynxtedition/node:pipeline-response-destroyCopy head branch name to clipboard

Conversation

ronag commented Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Dec 21, 2019

Uh oh!

szmarczak commented Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Dec 21, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 21, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 23, 2019

Uh oh!

BridgeAR Dec 24, 2019

Choose a reason for hiding this comment

Uh oh!

szmarczak Dec 24, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

Trott commented Dec 25, 2019

Uh oh!

targos commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

stream: pipeline should use req.abort() to destroy response#31054
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:pipeline-response-destroy

ronag commented Dec 21, 2019 •
edited
Loading

szmarczak commented Dec 21, 2019 •
edited
Loading

ronag Dec 25, 2019 •
edited
Loading

lpinca Dec 25, 2019 •
edited
Loading

lpinca Dec 25, 2019 •
edited
Loading

ronag Dec 25, 2019 •
edited
Loading