GeezerVison

CI: https://ci.nodejs.org/job/node-test-pull-request/13369/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1308/

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

@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.

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

@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.

@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

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.

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

@nodejs/streams @mafintosh PTAL

CI https://ci.nodejs.org/job/node-test-pull-request/13542/

@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.

Lots of good tests! LGTM from me 👍

CI: https://ci.nodejs.org/job/node-test-pull-request/13685/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1331/

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

CI: https://ci.nodejs.org/job/node-test-pull-request/14051/

@addaleax PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/14077/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1353/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1354/

Landed as cf5f986

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)
  ...

@MylesBorins We are already tracking it in #19905.