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

Readable stream with highWaterMark==0 behavior #5085

Closed
kanongil opened this Issue Mar 19, 2013 · 11 comments

Comments

Projects
None yet
3 participants
@kanongil

I have made a Readable object-stream which takes a highWaterMark option. This works fine for the most part, but I find that the Readable interaction is buggy and/or strange when highWaterMark==0.

With hwm==0, I would expect the stream to match any read() calls from a consumer with an internal _read() call. Currently I see two _read() calls before my first read() has returned, and the remaining _read() calls always seems ahead.

Expected flow from client:

  1. Client registers r.on('readable') which triggers a r._read().
  2. Readable triggers 'readable' from async r.push().
  3. Client calls read() which returns the pushed object.
  4. Client calls read() again which triggers r._read() and returns null since data is async.
  5. goto 2.
@kanongil

This comment has been minimized.

Show comment Hide comment
@kanongil

kanongil Apr 8, 2013

@isaacs Any comments on this? Is it a bug, or intended behavior?

kanongil commented Apr 8, 2013

@isaacs Any comments on this? Is it a bug, or intended behavior?

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Apr 9, 2013

So, I would think that (3) would trigger another _read. You've reduced the length to 0, which is <= highWaterMark. Is this not what you're seeing?

isaacs commented Apr 9, 2013

So, I would think that (3) would trigger another _read. You've reduced the length to 0, which is <= highWaterMark. Is this not what you're seeing?

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Apr 9, 2013

Can you show some code that does the wrong thing?

isaacs commented Apr 9, 2013

Can you show some code that does the wrong thing?

@kanongil

This comment has been minimized.

Show comment Hide comment
@kanongil

kanongil Apr 9, 2013

There is not anything special about the code, but you can look at it here along with a consumer.

Is it really intended that you can't configure the Readable to not read ahead? This seems like an oversight or bug to me. As I see it, there is no reason to call _read() until step 4, which would solve this.

kanongil commented Apr 9, 2013

There is not anything special about the code, but you can look at it here along with a consumer.

Is it really intended that you can't configure the Readable to not read ahead? This seems like an oversight or bug to me. As I see it, there is no reason to call _read() until step 4, which would solve this.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Apr 9, 2013

Hm. I see. So, the change in semantics would be that you read-ahead if we have < the hwm (and the hwm is not 0) OR that a previous read returned null, setting the 'needReadable' flag.

isaacs commented Apr 9, 2013

Hm. I see. So, the change in semantics would be that you read-ahead if we have < the hwm (and the hwm is not 0) OR that a previous read returned null, setting the 'needReadable' flag.

@kanongil

This comment has been minimized.

Show comment Hide comment
@kanongil

kanongil Apr 9, 2013

Not quite. If the previous read() returns null, it will be because _read() was called, but there was no sync push(), ie. a regular read with an empty buffer.

kanongil commented Apr 9, 2013

Not quite. If the previous read() returns null, it will be because _read() was called, but there was no sync push(), ie. a regular read with an empty buffer.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Apr 9, 2013

Right, I missaid that. Basically, doRead = needReadable || length < hwm

isaacs commented Apr 9, 2013

Right, I missaid that. Basically, doRead = needReadable || length < hwm

@kanongil

This comment has been minimized.

Show comment Hide comment
@kanongil

kanongil Apr 9, 2013

Yes, something like that.

kanongil commented Apr 9, 2013

Yes, something like that.

@jasnell

This comment has been minimized.

Show comment Hide comment
@jasnell

jasnell May 20, 2015

Owner

@isaacs ... was there ever any action on this? This looks like a feature request, yes?

Owner

jasnell commented May 20, 2015

@isaacs ... was there ever any action on this? This looks like a feature request, yes?

@kanongil

This comment has been minimized.

Show comment Hide comment
@kanongil

kanongil May 20, 2015

I just had another a look at this, and it appears that this has been fixed in streams3.

Test script:

var R = require('readable-stream').Readable;

var cnt = 0;
var r = new R({ highWaterMark: 0, objectMode: true });
r._read = function(n) {
  console.log('_read');
  setImmediate(function() {
    if (cnt > 1) return r.push(null);
    r.push(cnt++);
  });
};

r.on('readable', function() {
  var obj;
  while (null !== (obj = r.read())) {
    console.log('object', obj);
  }
});

Result using readable-stream@1.0.33:

_read
_read
object 0
_read
object 1

Result using readable-stream@1.1.13 (note the initial double _read is gone):

_read
object 0
_read
object 1
_read

I just had another a look at this, and it appears that this has been fixed in streams3.

Test script:

var R = require('readable-stream').Readable;

var cnt = 0;
var r = new R({ highWaterMark: 0, objectMode: true });
r._read = function(n) {
  console.log('_read');
  setImmediate(function() {
    if (cnt > 1) return r.push(null);
    r.push(cnt++);
  });
};

r.on('readable', function() {
  var obj;
  while (null !== (obj = r.read())) {
    console.log('object', obj);
  }
});

Result using readable-stream@1.0.33:

_read
_read
object 0
_read
object 1

Result using readable-stream@1.1.13 (note the initial double _read is gone):

_read
object 0
_read
object 1
_read
@jasnell

This comment has been minimized.

Show comment Hide comment
@jasnell

jasnell May 20, 2015

Owner

Great! Then I'll close. Can reopen if necessary

Owner

jasnell commented May 20, 2015

Great! Then I'll close. Can reopen if necessary

@jasnell jasnell closed this May 20, 2015

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