Repository navigation
The error handler isn't removed on successful stream async iteration #32995
Description
Activity
I realize that function eof callback didn't call up
diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js index 4742391fd7..f6cac90dc0 100644 --- a/lib/internal/streams/end-of-stream.js +++ b/lib/internal/streams/end-of-stream.js @@ -8,6 +8,7 @@ const { ERR_STREAM_PREMATURE_CLOSE } = require('internal/errors').codes; const { once } = require('internal/util'); +const debug = require('internal/util/debuglog').debuglog('stream'); function isRequest(stream) { return stream.setHeader && typeof stream.abort === 'function'; @@ -60,6 +61,7 @@ function eos(stream, opts, callback) { (opts.readable !== false && isReadable(stream)); const writable = opts.writable || (opts.writable !== false && isWritable(stream)); + debug('eos', readable, writable); const wState = stream._writableState; const rState = stream._readableState; @@ -152,6 +154,7 @@ function eos(stream, opts, callback) { } return function() { + debug('eos cleanup'); callback = nop; stream.removeListener('aborted', onclose); stream.removeListener('complete', onfinish);
\node\Release\node.exe C:\Users\Himself65\Desktop\github\test\1.js STREAM 21764: eos true false STREAM 21764: on readable 0 false STREAM 21764: read undefined STREAM 21764: need readable true STREAM 21764: length less than watermark true STREAM 21764: do read STREAM 21764: readableAddChunk chunk STREAM 21764: emitReadable true false STREAM 21764: emitReadable false STREAM 21764: readableAddChunk null STREAM 21764: onEofChunk STREAM 21764: emitReadable false true STREAM 21764: endReadable false STREAM 21764: readable nexttick read 0 STREAM 21764: read 0 STREAM 21764: endReadable false STREAM 21764: emitReadable_ false 0 true STREAM 21764: flow false STREAM 21764: endReadableNT false 0 STREAM 21764: endReadableNT true 0 1 Process finished with exit code 0
I didn't work on this part before so I don't know if it's a bug or a feature
and this appears on the versions which more than
v10cc @nodejs/streams
I think this is by design. Anything that uses
finishedorpipelinewill have a dangling handlers, e.g.error. This is stated in the documentation. Though maybe not for async iteration.Reacted by Alex YangReacted by Szymon MarczakYes, it's by design. Closing
Reacted by Szymon Marczak@himself65 It might be worth to add a corresponding section under async iterator:
stream.finished() leaves dangling event listeners (in particular 'error', 'end', 'finish' and 'close') after callback has been invoked. The reason for this is so that unexpected 'error' events (due to incorrect stream implementations) do not cause unexpected crashes. If this is unwanted behavior then the returned cleanup function needs to be invoked in the callback:
Reacted by Alex Yangyes, that's better. so I reopen this until PR fixes
Reacted by Robert NagyIf this is unwanted behavior then the returned cleanup function needs to be invoked in the callback
It should be possible to do so in all cases, including async iterators.
due to incorrect stream implementations
What incorrect stream implementations?
Can you link to a PR that introduced this change please?
It should be possible to do so in all cases, including async iterators.
I don't see how it would be possible with async iterators.
What incorrect stream implementations?
Those that emit
'error'after completion.24 remaining items
E.g. expose it via stream.cleanup()
How would this be different from:
stream.removeAllListeners('error')But we are talking about async iterator now?
Sorry, I do not follow.But async iterator utilizes streams, that's the thing. They mean almost 99% the same. You understand this already.
I guess, basically once you have converted a stream into an async iterator it should not really be used as a stream anymore.
So there shouldn't be a hanging
errorlistener.How? Once the for await loop completes or exits it should not be emitting any further errors.
Let me explain so you understand this correctly: the async iterator leaves a hanging
errorlistener. Once the response has been read via the async iterator, Got cannot reuse theerrorevent anymore to throw aHTTPErrorwhen it receives 404 for example. This breaks myhttp-timerpackage, which silently listens for theerrorevent. It would be required to expose another function to pass the error to thehttp-timerinstance. It can be avoided using this:some sort of an internal end event, so the algorithm knows when the response has been read and delays this.push(null) so the error event is emitted before the end one.
How would this be different from:
You cannot guarantee that someone didn't attach their own listener.
Got cannot reuse the error event anymore to throw a HTTPError when it receives 404 for example.
Re-use the error event? Are you doing
stream.emit('error', err)? That's not really supported by streams.Once the response has been read via the async iterator
Once the read of the async iterator is completed the stream is
destroy():d and awaited and should not be usable for anything else.It sounds to me like you are trying to use streams in a way it was not really intended.
You cannot guarantee that someone didn't attach their own listener.
Yes, so how would
stream.cleanup()be implemented?In my http-timer package I
You are overriding emit here. That's really a bit outside of supported usage. You are a bit on your own risk here.
I'm not really following along here. If you prefer maybe a google hangout call or something would be more constructive? Would help if I understand your use case better.
Are you doing stream.emit('error', err)?
Exactly.
It sounds to me like you are trying to use streams in a way it was not really intended.
Otherwise it would be necessary to delay the
endevent until the algorithm made sure no error will occur. This would also delay the callback forgotPromise.once('downloadProgress', ...), which could be a bit misleading.You are overriding emit here. That's really a bit outside of supported usage. You are a bit on your own risk here.
emitter[Symbol.for('nodejs.rejection')](err, eventName[, ...args])is experimental. I don't see any other solution as of now.Would help if I understand your use case better.
I know, sorry I haven't set up a more construcive example yet. Will do.
Exactly.
Yea, that kind of breaks some stream assumptions.
I know, sorry I haven't set up a more construcive example yet. Will do.
Cool. I'm more than happy to try and help out but digging into the details of got is a little outside my timeframe.
Otherwise it would be necessary to delay the end event
Could you wait for close instead?
Reacted by Szymon Marczak@szmarczak Regarding http-timer. You probably want to listen to
'aborted'on the response object.Also, have you looked at
EventEmitter.errorMonitor?Reacted by Szymon MarczakNice find, thanks :) I guess I look at the docs not so often, this definitely solves the
origin.emit = ...thing.I'm sketching some code for the original problem rn.
Yea, that kind of breaks some stream assumptions.
I think it would be more useful to throw on every action if the stream is destroyed to avoid ambiguity.
Cool. I'm more than happy to try and help out but digging into the details of got is a little outside my timeframe.
Thanks for taking your time. I really appreciate it!
Could you wait for close instead?
That would work too, definitely.
I would recommend closing this. The dangling
'error'listener is a design choice to prevent a "non compliant" stream implementation to crash the process without the user adding one.I'm closing.
Reacted by Robert Nagy@ronag I made an example of 237 lines (sorry that's the shortest example I came up with): https://gist.github.com/szmarczak/e7eb659bebb33bceb7577e90d7216aa3
The line I care the most is line no. 198.
Yes, so how would stream.cleanup() be implemented?
Call
stream.removeListener(name, function)on these:stream.finished() leaves dangling event listeners (in particular 'error', 'end', 'finish' and 'close') after callback has been invoked.
is a design choice
Have you actually encountered a package that benefits from this? Or was this just "it's possible, so let's do this"? Nevermind, let's forget this question. It doesn't change anything. But I'm strongly against the latter.
Sorry if my behavior was a bit harsh, I was 10000% convinced this was a bug and I didn't expect how it turned out to be.
Have you actually encountered a package that benefits from this? Or was this just "it's possible, so let's do this"? Nevermind, let's forget this question. It doesn't change anything. But I'm strongly against the latter.
A significant amount of "old/legacy" stream packages could emit multiple
'error'events.finishedis implemented to be safe, exactly for this reason. To add some context,finishedis an evolution of http://npm.im/end-of-stream, which is downloaded 15 million times per week. I would say that most of the NPM ecosystem benefit from this behavior.Reacted by Szymon Marczak
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
What do you see instead?
Additional information
The
errorhandler seems to be from https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js