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

lib: ensure readable stream flows to end #24918

Closed
wants to merge 8 commits into from

Conversation

Rantanen
Copy link
Contributor

@Rantanen Rantanen commented Dec 9, 2018

If a readable stream was set up with highWaterMark 0, the while-loop in maybeReadMore_ function would never execute.

The while loop now has an extra or-condition for the case where the stream is flowing and there are no items. The or-condition is adapted from the emit-condition of the addChunk function.

The addChunk also contains a check for state.sync. However that part of the check was omitted here because the maybeReadMore_ is executed using process.nextTick. state.sync is set and then unset within the read() function so it should never be in effect in maybeReadMore_.

Fixes: #24915

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: nodejs#24915
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Dec 9, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @nodejs/streams

lib/_stream_readable.js Outdated Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @nodejs/streams PTAL

@mcollina
Copy link
Member

lib/_stream_readable.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add another test that verifies the interaction between on('data') and on('readable') with this change?

@Rantanen
Copy link
Contributor Author

Rantanen commented Dec 11, 2018

There's one remaining issue in relation to the behavior documented in the readable._read documentation:

The size argument is advisory. For implementations where a "read" is a single operation that returns data can use the size argument to determine how much data to fetch. Other implementations may ignore this argument and simply provide data whenever it becomes available. There is no need to "wait" until size bytes are available before calling stream.push(chunk).

If the stream implementation (the _read method) has no data available and calls stream.push with a zero sized (non-null) chunk - which seems to be allowed based on the documentation - the len === state.length condition terminates the maybeReadMore loop and there will be no other mechanism for invoking the _read later in case the stream might have received more data.

In this case I feel it might be better to clarify the documentation by stating that push should not be called with an empty buffer. Although I'm not sure how common such implementation has been in the past, when most consumers would have been using the read() API which doesn't suffer from this issue (or leaves it for the consumer to solve).

The other option would probably be by doing a process.nextTick(maybeReadMore_, stream, state) in the break-branch where the while-loop is terminated. However I suspect this might result in the equivalent of a (yielding) busy loop if the stream is slow.

Edit: Also worth noting that this isn't limited to the highWaterMark: 0 case.

@Rantanen
Copy link
Contributor Author

Rantanen commented Dec 11, 2018

@mcollina How are they supposed to work together..? Did you have a specific scenario in mind?

I did some experimentation in subscribing readable, subscribing data, unsubscribing readable and then re-subscribing readable. It seems to work just fine - however I'm curious if you had some specific corner cases in mind that you'd like to have tests for?

(Running make -j4 test currently. Pushing the above test in once that finishes)

@Rantanen
Copy link
Contributor Author

Maybe next time I'll proof read my comments before pushing to avoid a pile of force pushes... :)

@mcollina Hopefully that test case is at least somewhat close to what you had in mind. I did author a bit more comprehensive test case (gist), but I don't believe asserting on the exact nextTick timings is that important so the one I pushed here just uses setTimeout to ensure no 'data' events are emitted without .read().

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit.

test/parallel/test-stream-readable-hwm-0-no-flow-data.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good work!

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2018
@mcollina
Copy link
Member

@mcollina
Copy link
Member

@Trott
Copy link
Member

Trott commented Dec 14, 2018

@Trott
Copy link
Member

Trott commented Dec 14, 2018

ARM problem looked host-specific. It had failed a dozen or so runs in a row. Took it offline. ARM CI again: https://ci.nodejs.org/job/node-test-commit-arm/20815/ ✔️

@Trott
Copy link
Member

Trott commented Dec 14, 2018

Landed in 37a5e01

@Trott Trott closed this Dec 14, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 14, 2018
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: nodejs#24915

PR-URL: nodejs#24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 18, 2018
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: #24915

PR-URL: #24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
@sholladay
Copy link
Contributor

Any chance this patch could be applied to the 10.x line? This fixes an important feature in one of my apps. I'm somewhat stuck using LTS versions and 12.x won't ship for a long time.

@mcollina
Copy link
Member

This will be backported to 10 in one of the next releases.

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: nodejs#24915

PR-URL: nodejs#24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: #24915

PR-URL: #24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: #24915

PR-URL: #24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
If a readable stream was set up with `highWaterMark 0`, the while-loop
in `maybeReadMore_` function would never execute.

The while loop now has an extra or-condition for the case where the
stream is flowing and there are no items. The or-condition is adapted
from the emit-condition of the `addChunk` function.

The `addChunk` also contains a check for `state.sync`. However that part
of the check was omitted here because the `maybeReadMore_` is executed
using `process.nextTick`. `state.sync` is set and then unset  within the
`read()` function so it should never be in effect in `maybeReadMore_`.

Fixes: #24915

PR-URL: #24918
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readable streams with highWaterMark: 0 terminate early
7 participants