The `error` handler isn't removed on successful stream async iteration

[szmarczak]

• Version: v13.13.0
• Platform: Linux solus 5.5.11-151.current deps: update openssl to 1.0.1j #1 SMP PREEMPT Tue Mar 24 18:06:46 UTC 2020 x86_64 GNU/Linux
• Subsystem: stream

What steps will reproduce the bug?

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

async function getBuffer(readable) {
	const chunks = [];

	for await (const chunk of readable) {
		chunks.push(chunk);
	}

	return Buffer.concat(chunks);
}

const stream = new Readable({
    read() {
        this.push('chunk');
        this.push(null);
    }
});

const buffer = await getBuffer(stream);

console.log(stream.listenerCount('error'));

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

0

What do you see instead?

1

Additional information

The error handler seems to be from https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js

[himself65]

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 v10

cc @nodejs/streams

[ronag]

I think this is by design. Anything that uses finished or pipeline will have a dangling handlers, e.g. error. This is stated in the documentation. Though maybe not for async iteration.

[himself65]

Yes, it's by design. Closing

Refs: https://nodejs.org/dist/latest-v14.x/docs/api/stream.html#stream_stream_finished_stream_options_callback

[ronag]

@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:

[himself65]

yes, that's better. so I reopen this until PR fixes

[szmarczak]

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

[szmarczak]

due to incorrect stream implementations

What incorrect stream implementations?

[szmarczak]

Can you link to a PR that introduced this change please?

[ronag]

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.