lib: ensure readable stream flows to end #24918
Conversation
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
added
the
stream
|
There's one remaining issue in relation to the behavior documented in the
If the stream implementation (the In this case I feel it might be better to clarify the documentation by stating that The other option would probably be by doing a Edit: Also worth noting that this isn't limited to the |
|
@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 |
|
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 |
mcollina
added
the
author ready
|
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/ ✔️ |
|
Landed in 37a5e01 |
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
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. |
|
This will be backported to 10 in one of the next releases. |
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
If a readable stream was set up with
highWaterMark 0, the while-loop inmaybeReadMore_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
addChunkfunction.The
addChunkalso contains a check forstate.sync. However that part of the check was omitted here because themaybeReadMore_is executed usingprocess.nextTick.state.syncis set and then unset within theread()function so it should never be in effect inmaybeReadMore_.Fixes: #24915
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes