nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.82k stars 29.71k forks source link

Stream not emitting end after removeListener #9002

Closed davedoesdev closed 8 years ago

davedoesdev commented 8 years ago

This program prints "END":

const s = new require('stream').PassThrough();

function readable()
{
    console.log(this.read());
}

s.end();

setTimeout(function ()
{
    s.on('readable', readable);
    s.on('end', function ()
    {
        console.log("END");
    });
}, 1000);

whereas this program does not:

const s = new require('stream').PassThrough();

function readable()
{
    console.log(this.read());
}

s.on('readable', readable);
s.removeListener('readable', readable);

s.end();

setTimeout(function ()
{
    s.on('end', function ()
    {
        console.log("END");
    });
}, 1000);

The only difference is that a readable listener is added and then removed before the stream is ended.

My question is whether the behaviour should be the same, given that there is no readable listener in both cases when the stream is ended.

I can see in the code why: At https://github.com/nodejs/node/blob/v6.7.0/lib/_stream_readable.js#L696 readableListening is set to true but it's never reset to false if all readable listeners are removed.

Before making any changes however, I'd like to ask whether this is intended behaviour?

mscdex commented 8 years ago

/cc @nodejs/streams

mcollina commented 8 years ago

hey @davedoesdev!

What you describe is the currently intended behavior. Setting on('readable') trigger as a side effect read(0). In fact, 'end' is emitted straight away, because the read was triggered.

This is what is triggered: https://github.com/nodejs/node/blob/v6.7.0/lib/_stream_readable.js#L710-L713

What you want is to "undo" that scheduled read(0) operation.  I'm not 100% convinced that we should support this. What's the use case for doing so?

There probably is a different bug about 'readableListening' not being reset to false. But fixing it would not prevent the behavior you are describing here.

davedoesdev commented 8 years ago

I have two separate bits of code (separate modules). One which reads a header from a stream and another which then reads the rest of the stream. The second bit of code when passed the stream (after the first has finished processing) then registers its own readable and end handlers. It needs to know when the stream has ended.

It's fine, I can get the second bit of code to check if the stream is already ended (using _readableState I guess).

mcollina commented 8 years ago

That is probably the best approach, yes.

davedoesdev commented 8 years ago

Thanks for the help @mcollina