Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Readable._read is called multiple times synchronously #24919

Closed
Rantanen opened this issue Dec 9, 2018 · 2 comments
Closed

Readable._read is called multiple times synchronously #24919

Rantanen opened this issue Dec 9, 2018 · 2 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@Rantanen
Copy link
Contributor

Rantanen commented Dec 9, 2018

  • Version: v10.10.0, v11.4.0
  • Platform: Linux 4.4.0-45-generic deprecate domains #66-Ubuntu SMP Wed Oct 19 14:12:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: stream

The documentation for readable._read(size) states the following:

readable._read() is guaranteed to be called only once within a synchronous execution, i.e. a microtick.

However the following example demonstrates starvation if that method is able to push all of its data synchronously. The stream implementation ends up calling the read-function repeatedly and won't yield execution as documented.

var Readable = require('stream').Readable;

let c = 10;
let r = new Readable({
    read: () => {
        process.nextTick(console.log, 'next-tick');
        r.push('a');
    }
});

r.on('data', () => {});
process.nextTick(console.log, 'Got food');  // Never printed.
console.log('Requested food');

I believe the issue is caused by the flow function calling stream.read repeatedly within a while-loop until the stream fails to return data:

function flow(stream) {
const state = stream._readableState;
debug('flow', state.flowing);
while (state.flowing && stream.read() !== null);
}

One option for fixing this might be to make flow recurse with next tick:

function flow(stream) {
  const state = stream._readableState;
  debug('flow', state.flowing);
  flow_(stream, state);
}

function flow_(stream, state) {
  if(state.flowing && stream.read() !== null)
    process.nextTick(flow_, stream, state);
}

Alternatively just updating the documentation might also work. I guess most streams that would encounter the issue would just delay r.push() into the next tick as a workaround.

Even in my case this was just something I spotted while looking into #24918. I don't personally have a use case for such a stream.

@hiroppy
Copy link
Member

hiroppy commented Jan 2, 2019

/cc @nodejs/streams

@hiroppy hiroppy added the stream Issues and PRs related to the stream subsystem. label Jan 2, 2019
mcollina added a commit to mcollina/node that referenced this issue Jan 11, 2019
nodejs#17979 introduced a change in the
doc that was not correct about _read always being called asynchronously.
This does not hold true when it is in flowing mode.

See: nodejs#17979
Fixes: nodejs#24919
@mcollina
Copy link
Member

Fixed in 2f1ae9e.

mcollina added a commit that referenced this issue Jan 15, 2019
#17979 introduced a change in the
doc that was not correct about _read always being called asynchronously.
This does not hold true when it is in flowing mode.

See: #17979
Fixes: #24919

PR-URL: #25442
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
addaleax pushed a commit that referenced this issue Jan 15, 2019
#17979 introduced a change in the
doc that was not correct about _read always being called asynchronously.
This does not hold true when it is in flowing mode.

See: #17979
Fixes: #24919

PR-URL: #25442
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
nodejs#17979 introduced a change in the
doc that was not correct about _read always being called asynchronously.
This does not hold true when it is in flowing mode.

See: nodejs#17979
Fixes: nodejs#24919

PR-URL: nodejs#25442
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants