stream: first error wins and cannot be overriden #30982

ronag · 2019-12-15T17:45:16Z

The first stream error is the only one that gets emitted as 'error' or forwarded through callbacks. Also it cannot be overriden or swallowed by _destroy. This is in order to easier reason what will happen in regards to streams and errors. Currently it's quite difficult to know what to expect in different cases.

See #30979 for further discussion.

destroy(err, cb), i.e. destroy with callback is now also able to actually override the error (see https://github.com/nodejs/node/pull/30982/files#r357996368). I don't believe this affects anything in practice.

semver-major

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 · 2019-12-15T17:45:40Z

ping @lpinca

The first stream error is the only one that gets emitted as 'error' or forwarded in callbacks. Also it cannot be override by _destroy. Refs: nodejs#30979

ronag · 2019-12-15T18:25:08Z

test/parallel/test-stream-writable-destroy.js

@@ -249,6 +276,9 @@ const assert = require('assert');
  write.destroy(expected, common.mustCall(function(err) {


destroy(err, cb)is now able to override the no error state from destroy() above.

ronag · 2019-12-15T18:37:25Z

@addaleax: This fails in test-gc-tls-external-memory.js which is a bit over my head. Could I ask for some assistance in regards to what might cause a failure there? Doesn't seem to be a flaky test.

ronag · 2019-12-15T18:46:55Z

test/parallel/test-stream-auto-destroy.js


  r.pipe(r2);
 }

 {
  const r = new stream.Readable({
    read() {
-      w.emit('error', new Error('fail'));
+      w.destroy(new Error('fail'));


Please note this change. Calling emit('error' is not something that should be done. But do we need to support it?

ronag · 2019-12-15T18:46:58Z

test/parallel/test-stream-auto-destroy.js

@@ -86,27 +86,29 @@ const assert = require('assert');
 {
  const r = new stream.Readable({
    read() {
-      r2.emit('error', new Error('fail'));
+      r2.destroy(new Error('fail'));


Please note this change. Calling emit('error' is not something that should be done. But do we need to support it?

lpinca · 2019-12-15T19:09:32Z

@nodejs/streams

mscdex · 2019-12-15T19:42:56Z

s/overriden/overridden/ in commit message

nodejs-github-bot · 2019-12-15T20:04:34Z

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

mcollina

I'm -1 for this change. The purpose of the err parameter in _destroy() is to be able to change/handle the errors.

I'm not sure what is the context of this change and I don't get how #30979 lead to this.

ronag · 2019-12-15T20:36:32Z

The purpose of the err parameter in _destroy() is to be able to change/handle the errors.

But that doesn't really work in practice? e.g.

const { Readable } = require('stream');

stream = new Readable({
  read() {},
  destroy (err, cb) {
    setTimeout(() => {
      cb(new Error('other error'));
    }, 100);
  }
})
stream.destroy(new Error('error 1'));
stream.destroy(new Error('error 2'));
stream.on('error', (err) => {
  console.log(err.message); // prints 'error 2', confusion
});

lpinca · 2019-12-15T20:40:33Z

@ronag and that is "ok" what is not ok in my opinion is that you can't swallow the second error even if you defined a custom _destroy() to do that which is what we discussed in #30979.

ronag · 2019-12-15T20:47:38Z

I do not have a better suggestion. The only other way I can think of to make this behave in a consistent manner is to ignore any destroy(err) (including the error) once _destroy has been invoked, since we can't call _destroy multiple times to consistently apply the error handling logic.

ronag · 2019-12-16T09:28:24Z

ignore any destroy(err) (including the error) once _destroy has been invoked, since we can't call _destroy multiple times to consistently apply the error handling logic.

I actually kind of like this option. Doing it like this would make things a lot easier both in terms of implementation and user complexity. It kind of goes back to the discussion in #29197 where we originally discussed not allowing any errors after destroy().

lpinca · 2019-12-16T09:34:29Z

ignore any destroy(err) (including the error) once _destroy has been invoked

-1, in this case I want the error to surface if the _destroy() callback is called without an error as discussed in #29197

ronag · 2019-12-16T10:01:43Z

-1, in this case I want the error to surface if the _destroy() callback is called without an error as discussed in #29197

That would be something like?

error returned from _destroy always wins.
if no error was passed to _destroy and a destroy(err) was invoked during the destruction, then that error is applied.
if destroy(err) is invoked after 'close' that error gets ignored.

@lpinca: One problem with this is:

_destroy is supposed to swallow error
_destroy is called without error
destroy is called with error
we expect all errors to be swallowed, however the secondary destroy(err) will surface since _destroy was originally called without error.

lpinca · 2019-12-16T14:25:30Z

Yes which brings us back to #30979. We are running in circles. I like the simplicity and predictability of the solution proposed here.

ronag · 2019-12-16T16:03:24Z

Yes, you are right. As far as I see the current situation is:

First error wins (this PR & custom _destroy can swallow error? #30979). @mcollina -1
First destroy() wins (as originally discussed in stream: fix multiple destroy() calls #29197). @lpinca -1
Leave as is. @ronag -1

Fishrock123 · 2019-12-16T19:50:07Z

I think I was most comfortable with the approach in #29197... Or at least it makes sense to me that destroy(err) would be the priority.

ronag · 2019-12-16T23:46:21Z

@mcollina: as we seem a bit stuck with this would it make sense to raise this to TSC? It's a bit re-occuring and touches a few different areas. If required I could write a short summary of the discussed issues and solutions based on #30982, #29197, #30979, #30970, #30906. The question is which direction to go amongst #30982 (comment) (or something else).

mcollina · 2019-12-17T00:11:05Z

I think that would be helpful, I’m happy to review that document before it’s shared with everybody.

lpinca · 2019-12-18T15:13:10Z

If this is not acceptable my vote is for keeping things as is. In my opinion #30979 is an edge case that is probably not worth fixing.

I think that custom _destroy() implementations that swallow errors are not very common and they might swallow only some errors depending on the error itself. In a such case if stream.destroy(err) is called again and _destroy() callback was not called should err be swallowed? Since _destroy() is not called again there is no way to know if err is one of the errors to ignore.

ronag · 2019-12-22T17:53:34Z

Closing this for now pending #31060

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Dec 15, 2019

stream: first error wins and cannot be overriden

2c96024

The first stream error is the only one that gets emitted as 'error' or forwarded in callbacks. Also it cannot be override by _destroy. Refs: nodejs#30979

ronag force-pushed the stream-error-first branch from 4af24a5 to 2c96024 Compare December 15, 2019 18:15

ronag commented Dec 15, 2019

View reviewed changes

fixup: TODO

339aa07

ronag force-pushed the stream-error-first branch from 17b34a4 to 339aa07 Compare December 15, 2019 18:40

ronag commented Dec 15, 2019

View reviewed changes

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 15, 2019

lpinca approved these changes Dec 15, 2019

View reviewed changes

mcollina requested changes Dec 15, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

ronag mentioned this pull request Dec 22, 2019

stream: destroy(err) predictability #31060

Closed

ronag closed this Dec 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: first error wins and cannot be overriden #30982

stream: first error wins and cannot be overriden #30982

ronag commented Dec 15, 2019 •

edited

Loading

ronag commented Dec 15, 2019

ronag Dec 15, 2019 •

edited

Loading

ronag commented Dec 15, 2019 •

edited

Loading

ronag Dec 15, 2019 •

edited

Loading

ronag Dec 15, 2019 •

edited

Loading

lpinca commented Dec 15, 2019

mscdex commented Dec 15, 2019

nodejs-github-bot commented Dec 15, 2019

mcollina left a comment

ronag commented Dec 15, 2019 •

edited

Loading

lpinca commented Dec 15, 2019 •

edited

Loading

This comment has been minimized.

ronag commented Dec 15, 2019 •

edited

Loading

ronag commented Dec 16, 2019 •

edited

Loading

lpinca commented Dec 16, 2019 •

edited

Loading

ronag commented Dec 16, 2019

lpinca commented Dec 16, 2019

ronag commented Dec 16, 2019 •

edited

Loading

Fishrock123 commented Dec 16, 2019

This comment has been minimized.

ronag commented Dec 16, 2019 •

edited

Loading

mcollina commented Dec 17, 2019

lpinca commented Dec 18, 2019

ronag commented Dec 22, 2019

		@@ -249,6 +276,9 @@ const assert = require('assert');
		write.destroy(expected, common.mustCall(function(err) {

stream: first error wins and cannot be overriden #30982

stream: first error wins and cannot be overriden #30982

Conversation

ronag commented Dec 15, 2019 • edited Loading

Checklist

ronag commented Dec 15, 2019

ronag Dec 15, 2019 • edited Loading

Choose a reason for hiding this comment

ronag commented Dec 15, 2019 • edited Loading

ronag Dec 15, 2019 • edited Loading

Choose a reason for hiding this comment

ronag Dec 15, 2019 • edited Loading

Choose a reason for hiding this comment

lpinca commented Dec 15, 2019

mscdex commented Dec 15, 2019

nodejs-github-bot commented Dec 15, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Dec 15, 2019 • edited Loading

lpinca commented Dec 15, 2019 • edited Loading

This comment has been minimized.

ronag commented Dec 15, 2019 • edited Loading

ronag commented Dec 16, 2019 • edited Loading

lpinca commented Dec 16, 2019 • edited Loading

ronag commented Dec 16, 2019

lpinca commented Dec 16, 2019

ronag commented Dec 16, 2019 • edited Loading

Fishrock123 commented Dec 16, 2019

This comment has been minimized.

ronag commented Dec 16, 2019 • edited Loading

mcollina commented Dec 17, 2019

lpinca commented Dec 18, 2019

ronag commented Dec 22, 2019

ronag commented Dec 15, 2019 •

edited

Loading

ronag Dec 15, 2019 •

edited

Loading

ronag commented Dec 15, 2019 •

edited

Loading

ronag Dec 15, 2019 •

edited

Loading

ronag Dec 15, 2019 •

edited

Loading

ronag commented Dec 15, 2019 •

edited

Loading

lpinca commented Dec 15, 2019 •

edited

Loading

ronag commented Dec 15, 2019 •

edited

Loading

ronag commented Dec 16, 2019 •

edited

Loading

lpinca commented Dec 16, 2019 •

edited

Loading

ronag commented Dec 16, 2019 •

edited

Loading

ronag commented Dec 16, 2019 •

edited

Loading