This repository has been archived by the owner. It is now read-only.

Missing "readable" event after some IO has occurred #5141

Closed
mjackson opened this Issue Mar 26, 2013 · 7 comments

Comments

Projects
None yet
2 participants
@mjackson

This issue is a continuation of a discussion that we're having on the mailing list.

When I get a new IncomingMessage from an http.get, and I wait for a bit, I consistently miss the readable event even though there are no other listeners already registered for that event. The following example illustrates the issue I'm seeing on node 0.10.1:

var http = require('http');

http.get('http://joyent.com/', function (response) {
  console.log('got response with status ' + response.statusCode);
  console.log('number of readable listeners: ' + response.listeners('readable').length);

  setTimeout(function () {
    response.on('readable', function () {
      console.log('readable');
    });

    response.on('end', function () {
      console.log('end');
    });
  }, 10);
});

As you can see from the example, I'm checking to make sure that there's nobody else listening for the readable event, so the stream shouldn't have already emitted it before I setup my handler inside the timeout. But I never get the readable event at all, or the end event. Instead, the process just exits.

The odd thing about this code is that I can sometimes get the readable event to fire if I use a very small timeout (like 1ms).

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 26, 2013

Aha!!! This is an interesting edge case.

My point on the mailing list is that you don't have to do stream.read(0) to start the flow - adding a readable handler is enough. However, if the stream has already done some stream.push(chunk) then it already emitted a readable event, and you'll miss it. I was overlooking that case.

I consider this a bug, and I think it's easy to fix.

isaacs commented Mar 26, 2013

Aha!!! This is an interesting edge case.

My point on the mailing list is that you don't have to do stream.read(0) to start the flow - adding a readable handler is enough. However, if the stream has already done some stream.push(chunk) then it already emitted a readable event, and you'll miss it. I was overlooking that case.

I consider this a bug, and I think it's easy to fix.

@mjackson

This comment has been minimized.

Show comment Hide comment
@mjackson

mjackson Mar 26, 2013

@isaacs <3 thanks!

FWIW I took a stab at this last night. I tried setting the needReadable flag to true inside the readable.on handler when the event type is readable, but it didn't get me anywhere. After digging around a bit I found that the readable event was being triggered by someone else calling stream.push(chunk), so you're right about that.

@isaacs <3 thanks!

FWIW I took a stab at this last night. I tried setting the needReadable flag to true inside the readable.on handler when the event type is readable, but it didn't get me anywhere. After digging around a bit I found that the readable event was being triggered by someone else calling stream.push(chunk), so you're right about that.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Mar 26, 2013

stream: Handle late 'readable' event listeners
In cases where a stream may have data added to the read queue before the
user adds a 'readable' event, there is never any indication that it's
time to start reading.

True, there's already data there, which the user would get if they
checked However, as we use 'readable' event listening as the signal to
start the flow of data with a read(0) call internally, we ought to
trigger the same effect (ie, emitting a 'readable' event) even if the
'readable' listener is added after the first emission.

To avoid confusing weirdness, only the *first* 'readable' event listener
is granted this privileged status.  After we've started the flow (or,
alerted the consumer that the flow has started) we don't need to start
it again.  At that point, it's the consumer's responsibility to consume
the stream.

Closes #5141

mjackson added a commit to mjackson/node that referenced this issue Mar 26, 2013

Add failing test
A stream should emit "readable" when it gets an EOF but has not yet
passed its highWaterMark.

Re: nodejs/node-v0.x-archive#5141
@mjackson

This comment has been minimized.

Show comment Hide comment
@mjackson

mjackson Mar 26, 2013

Thanks! I can confirm that the test cases you added to address this specific issue are passing.

However, we're also missing readable events when a stream has not yet reached its highWaterMark but has received an EOF. I'm not sure if that's supported or not, but it seems like it should be given the way streams already work. I added a failing test and am happy to discuss this in a separate issue if you'd like.

Thanks! I can confirm that the test cases you added to address this specific issue are passing.

However, we're also missing readable events when a stream has not yet reached its highWaterMark but has received an EOF. I'm not sure if that's supported or not, but it seems like it should be given the way streams already work. I added a failing test and am happy to discuss this in a separate issue if you'd like.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 27, 2013

Awesome, thanks for the failing test. I'll check that out and update the pull, probably tomorrow.

isaacs commented Mar 27, 2013

Awesome, thanks for the failing test. I'll check that out and update the pull, probably tomorrow.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 27, 2013

@mjijackson Failing test fixed on 582c5b1. It's sufficiently different that it should be a separate commit, imo. Added to pull req.

isaacs commented Mar 27, 2013

@mjijackson Failing test fixed on 582c5b1. It's sufficiently different that it should be a separate commit, imo. Added to pull req.

isaacs added a commit that referenced this issue Mar 28, 2013

stream: Handle late 'readable' event listeners
In cases where a stream may have data added to the read queue before the
user adds a 'readable' event, there is never any indication that it's
time to start reading.

True, there's already data there, which the user would get if they
checked However, as we use 'readable' event listening as the signal to
start the flow of data with a read(0) call internally, we ought to
trigger the same effect (ie, emitting a 'readable' event) even if the
'readable' listener is added after the first emission.

To avoid confusing weirdness, only the *first* 'readable' event listener
is granted this privileged status.  After we've started the flow (or,
alerted the consumer that the flow has started) we don't need to start
it again.  At that point, it's the consumer's responsibility to consume
the stream.

Closes #5141
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 28, 2013

Fixed on eafa902 and 929e4d9. Thanks!

isaacs commented Mar 28, 2013

Fixed on eafa902 and 929e4d9. Thanks!

@mjackson

This comment has been minimized.

Show comment Hide comment
@mjackson

mjackson Apr 3, 2013

Thanks @isaacs! Sorry for the slow response.

mjackson commented Apr 3, 2013

Thanks @isaacs! Sorry for the slow response.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.