-
Notifications
You must be signed in to change notification settings - Fork 30k
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: fix end-of-stream for silent .destroy()
#26638
Conversation
Each stream can get `emitClose` option which, when set in `false` will prevent stream to emit `close` event in case if `.destroy()` was called without error argument. If this stream is passed as a last element in streams aray in `.pipeline()` method it could lead to a problem because `end-of-stream` won't be able to recognize that stream has been ended. This can be fixed by checking if this option is specified and overriding `_destroy()` method in order to be aware of this method has been called. Fixes: #26550
cc @nodejs/streams |
Similar to #24926 |
cb(_err); | ||
}); | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this is the correct fix. We should not monkey patching the incoming stream. If the default implementation of destroy needs a change, let's do it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina do you think some private method like _onDestroy
would make sense?
Or what you think is the best way to proceed with this and be notified when this silent destroy is happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the main problem here is that currently it's allowed for stream to be silently destroyed, but we can not change this behaviour now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stream should not be tampered with in any form apart from adding events. This is a hard rule, this function is not expected to alter the internals in the streams. We should also not rely on internals as much as possible, as "previous generation" streams could be passed inside these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can also think about introducing new “destroy” event which will be always emitted after “.destroy()” method.
Would it be the better way? My initial idea was to touch the smallest piece of code and functionality as possible. Each stream has “.destroy()” method so it seems natural to monkey patch it. In this way we don’t even need to introduce something new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end-of-stream cannot catch those because emitClose: false
prevent them to be observable. That's the whole reason for that option - it's a "you should handle this yourself if you want to, these are dangerous waters" option. Now, the documentation might be improved..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does stdout set emitClose to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net
set it to false because it handles emit('close')
in its own _destroy
method. However in
node/lib/internal/process/stdio.js
Line 17 in 1cdeb9f
stdout._destroy = dummyDestroy; |
_destroy
, and that means 'close'
is never emitted.I have a PR coming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generically a stream that sets emitClose: false
is responsible for emitting close in user-logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that sounds reasonable!
@tadjik1 I would need to check bug in detail to see how to look for a different fix. It does not look like this should happen, as both |
@mcollina absolutely! I left some tests so you can reproduce this bug. Unfortunately “end” as well as “finish” events can not help in this case because they both mean that data was fully consumed, which is not the case when “.destroy()” called. |
@mcollina consider this example:
is this desired behaviour of |
not needed as of #26638 (comment) |
Each stream can get
emitClose
option which, when setin
false
will prevent stream to emitclose
event incase if
.destroy()
was called without error argument.If this stream is passed as a last element in streams
aray in
.pipeline()
method it could lead to a problembecause
end-of-stream
won't be able to recognize thatstream has been ended.
For example:
This can be fixed by checking if this option is
specified and overriding
_destroy()
method in orderto be aware of this method has been called.
Fixes: #26550
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes