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

stream: 'readable' have precedence over flowing #18994

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

In Streams3 the 'readable' event/.read() method had a lower precedence
than the 'data' event that made them impossible to use them together.
This make .resume() a no-op if there is a listener for the
'readable' event, making the stream non-flowing if there is a
'data'  listener.

Fixes: #18058

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

stream, http

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 26, 2018
@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. dont-land-on-v4.x labels Feb 26, 2018
@mcollina
Copy link
Member Author

mcollina commented Feb 26, 2018

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2018

Wouldn't removeAllListeners() to be handled as well, since there could be no 'removeListener' event handlers?

@mcollina
Copy link
Member Author

@mscdex you are right. I didn't account for https://github.com/nodejs/node/blob/master/lib/events.js#L324-L336. Do you think I can just listen for 'removeListener' instead? I thought this approach could theoretically be more performing, but it's an edge case anyway.

@mafintosh
Copy link
Member

@mcollina so basically just suspends flowing when readable is set?

@mcollina
Copy link
Member Author

@mafintosh that's the goal yes. It also restores the behavior after 'readable' is removed.
I'm not sure this is something we want to land in this form (or we might want to refactor).
This is the fix for #18058.

@mafintosh
Copy link
Member

@mcollina wondering if we should even fix this. the same behaivor described in the issue would happen if there are two consumers of the readable event anyway (which is what happens in flowing mode). not too much of a fan of implicit event listener side effects

@mcollina
Copy link
Member Author

I am good with not fixing it. But it’s a discussion to have.

This fixed a major usability problem if you want to have both a on(‘data’) and on(‘readable’)/read() at the same time (with pipe) read() will always return null.

I’m ok if we want to remove the resume side effect when removing ‘readable’, as that might be confusing. However we should really be updating readableListening: this is also a bug without the fix for read() itself.

@mafintosh
Copy link
Member

Removing the removeListener stuff but keeping the other part sounds like a good middleway to me 👍

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

@nodejs/streams @mafintosh PTAL

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

@mcollina
Copy link
Member Author

mcollina commented Mar 7, 2018

@nodejs/tsc what do you think?

An alternative approach might be to remove the readableListening variable and just use listenerCount, hopefully it would be fast enough.

@@ -801,6 +811,10 @@ In general, the `readable.pipe()` and `'data'` event mechanisms are easier to
understand than the `'readable'` event. However, handling `'readable'` might
result in increased throughput.

If both `'readable'` and [`'data'`][] are used at the same time, `'readable'`
takes precedence in controlling the flow, i.e. `'data'` will be emitted
only when [`stream.read()`][stream-read] is called
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: missing period.

@@ -35,6 +35,7 @@ let expectEndingData = expectTotalData;
const r = new Readable({ highWaterMark: 1000 });
let chunks = totalChunks;
r._read = function(n) {
console.log('_read called', chunks);
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to add these logs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they follow the prevailing pattern in this test file. Without the console.log statements, this is undebuggable.

@mafintosh
Copy link
Member

Lots of good tests! LGTM from me 👍

@mcollina mcollina force-pushed the readable-and-data branch from 03e66ed to 44fad80 Compare March 14, 2018 21:17
@mcollina
Copy link
Member Author

@mcollina mcollina requested review from lpinca and a team March 14, 2018 21:28
@mcollina
Copy link
Member Author

Tagging @nodejs/tsc because it is semver-major.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 4, 2018
@@ -770,10 +774,16 @@ cause some amount of data to be read into an internal buffer.

```javascript
const readable = getReadableStreamSomehow();
readable.on('readable', () => {
readable.on('readable', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: not sure why this was changed and it doesn't really matter but for consistency I would keep the arrow function.

Copy link
Member

Choose a reason for hiding this comment

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

@lpinca it is using this.read() below

Copy link
Member

Choose a reason for hiding this comment

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

Oops, ignore me.

@mcollina mcollina force-pushed the readable-and-data branch from 44fad80 to 8085e2e Compare April 4, 2018 15:58
@mcollina mcollina added this to the 10.0.0 milestone Apr 4, 2018
@mcollina
Copy link
Member Author

mcollina commented Apr 4, 2018

@Trott Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 4, 2018
- version: REPLACEME
pr-url: FILLMEIN
description: >
using 'readable' requires calling .read().
Copy link
Member

Choose a reason for hiding this comment

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

Can you avoid the line break and capitalize the sentence?

- version: REPLACEME
pr-url: FILLMEIN
description: >
resume has no effect if there is a 'readable' event listening
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -223,6 +223,7 @@ Readable.prototype.unshift = function(chunk) {
};

function readableAddChunk(stream, chunk, encoding, addToFront, skipChunkCheck) {
debug('readableAddChunk');
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to have chunk printed out here as well


if (ev === 'data') {
// update readableListening
state.readableListening = this.listenerCount('readable') > 0;
Copy link
Member

Choose a reason for hiding this comment

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

Can you change to comment above this line so that it says why we do this, rather than what we do? I assume it’s about the this.resume() call below?

In Streams3 the 'readable' event/.read() method had a lower precedence
than the `'data'` event that made them impossible to use them together.
This make `.resume()` a no-op if there is a listener for the
`'readable'` event, making the stream non-flowing if there is a
`'data'`  listener.

Fixes: nodejs#18058
@mcollina mcollina force-pushed the readable-and-data branch from 8085e2e to 578a7b9 Compare April 4, 2018 21:11
@mcollina
Copy link
Member Author

mcollina commented Apr 4, 2018

@addaleax PTAL

@mcollina
Copy link
Member Author

mcollina commented Apr 6, 2018

Landed as cf5f986

@mcollina mcollina closed this Apr 6, 2018
@mcollina mcollina deleted the readable-and-data branch April 6, 2018 12:17
mcollina added a commit that referenced this pull request Apr 6, 2018
In Streams3 the 'readable' event/.read() method had a lower precedence
than the `'data'` event that made them impossible to use them together.
This make `.resume()` a no-op if there is a listener for the
`'readable'` event, making the stream non-flowing if there is a
`'data'`  listener.

Fixes: #18058

PR-URL: #18994
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

Seems like failures in cf5f986#diff-573b8412079b987e160a3fde511d5b9b are showing up in a couple places

In a couple CI runs in #19201 and also in http://github.com/nodejs/node/pull/19924

@mcollina thoughts?

not ok 750 parallel/test-http-readable-data-event
  ---
  duration_ms: 0.725
  severity: fail
  stack: |-
    assert.js:79
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: 'Hello World!Hello again later!' strictEqual 'Hello World!'
        at IncomingMessage.res.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/parallel/test-http-readable-data-event.js:43:14)
        at IncomingMessage.<anonymous> (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/common/index.js:467:15)
        at IncomingMessage.emit (events.js:182:13)
        at IncomingMessage.Readable.read (_stream_readable.js:489:10)
        at IncomingMessage.res.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/parallel/test-http-readable-data-event.js:36:20)
        at IncomingMessage.<anonymous> (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/common/index.js:467:15)
        at IncomingMessage.emit (events.js:182:13)
        at emitReadable_ (_stream_readable.js:537:12)
        at process._tickCallback (internal/process/next_tick.js:174:19)
  ...

@mcollina
Copy link
Member Author

@MylesBorins We are already tracking it in #19905.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.