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 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 ronag force-pushed the nxtedition:pipeline-response-destroy branch from b4a4544 to 16b4ed4 Dec 21, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

@szmarczak

This comment has been minimized.

Copy link
Contributor

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 6480882.

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

Fixes: #31029
Refs: 6480882
@ronag ronag force-pushed the nxtedition:pipeline-response-destroy branch from 16b4ed4 to 6dd8500 Dec 21, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

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

@lpinca
lpinca approved these changes Dec 21, 2019
test/parallel/test-stream-pipeline.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

mcollina left a comment

lgtm

@Qard
Qard approved these changes Dec 22, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Dec 23, 2019
@Trott Trott added the author ready label Dec 23, 2019
res,
stream,
common.mustCall((err) => {
server.close();

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Dec 24, 2019

Member

Does it make sense to also validate the error here?

This comment has been minimized.

Copy link
@szmarczak

szmarczak Dec 24, 2019

Contributor

I agree. At least that err is truthy.

This comment has been minimized.

Copy link
@ronag

ronag Dec 25, 2019

Author Member

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.

This comment has been minimized.

Copy link
@lpinca

lpinca Dec 25, 2019

Member

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

This comment has been minimized.

Copy link
@lpinca

lpinca Dec 25, 2019

Member

Bisecting points to 67ed526.

This comment has been minimized.

Copy link
@lpinca

lpinca Dec 25, 2019

Member

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

This comment has been minimized.

Copy link
@ronag

ronag Dec 25, 2019

Author Member

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

This comment has been minimized.

Copy link
@mcollina

mcollina Dec 25, 2019

Member

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.

This comment has been minimized.

Copy link
@ronag

ronag Dec 25, 2019

Author Member

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.

This comment has been minimized.

Copy link
@lpinca

lpinca Dec 25, 2019

Member

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 left a comment

lgtm

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 25, 2019

Landed in c852f7e

Trott added 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 <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>
@Trott Trott closed this Dec 25, 2019
BridgeAR added 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 <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>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@targos targos mentioned this pull request Jan 14, 2020
3 of 4 tasks complete
@targos

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.