Skip to content
New issue

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

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

Already on GitHub? Sign in 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants