Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Readable stream with highWaterMark==0 behavior #5085

Closed
kanongil opened this Issue · 11 comments

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

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

@isaacs

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

Can you show some code that does the wrong thing?

@kanongil

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

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

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

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

@kanongil

Yes, something like that.

@jasnell
Owner

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

@kanongil

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
Owner

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

@jasnell jasnell closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.