stream: restore flow if there are 'data' handlers after once('readable')

[mcollina]

In #18994, we made 'readable'  take precedence over 'data'/resume() and pause(). However, we didn't take into account situation where both 'readable'  and 'data' event handler were present at the same time. This PR addresses it by starting flowing after 'readable' is removed.

Fixes: #21398

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

[nodejs-github-bot]

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/472/pipeline

[mcollina]

This is an alternative implementation of #21696.
cc @lundibundi

[mcollina]

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

[lundibundi]

It seems like this one has the same flaws my PR had at the beginning

• ignoring user pause
• piping with .on('readable') fails

But the first one is documented now.
So basically there is no way to pause stream if you use 'readable' and pipe doesn't work. The latter is as before but I don't think it's a good behavior, though if we go with this PR I think it has to be documented explicitly in .pipe doc, as well as that stream will be set to paused if you add 'data' listener after 'readable'.

[mcollina]

ignoring user pause

That works exactly as expected. If resume() is ignored, why pause()  shouldn't?
stream.pause() and stream.resume() are methods to be used with .on('data').

piping with on('readable') fails

Which is as expected. on('readable') is a method of consuming data from a stream with backpressure, so a user has to consume it via stream.read() calls. Side note, without #18994 stream.read() always returned null when used with stream.pipe().

[lundibundi]

Yeah, this may be better as it simplifies the implementation a bit and signifies the fact that you shouldn't use 'readable' with 'data'. But as I said we have to better document it as issues of incorrect usage of streams are constantly popping up.

[mcollina]

I'll add some more docs for review tomorrow.

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

[mcollina]

@lundibundi @mafintosh PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/16338/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1499/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1500/

[vsemozhetbyt]

[`readable.read()`][]-> [`readable.read()`][stream-read]?

[vsemozhetbyt]

[`data`][] -> [`'data'`][]?

[mcollina]

CITGM seems ok.

[mcollina]

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

[mcollina]

@vsemozhetbyt PTAL

[vsemozhetbyt]

Doc part LGTM.

[mcollina]

@mafintosh @addaleax PTAL.

[mcollina]

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

[oyyd]

@mcollina I think this relates to #21122. What if we don't call read() inside the callback of 'readable' ? This might be regarded as an undefined behavior but calling on('data') before on('readable') like below:

const fs = require('fs')
const { resolve } = require('path')

const source = resolve(__dirname, 'index.js')
const target = resolve(__dirname, 'copy.js')

const reader = fs.createReadStream(source)
const writer = fs.createWriteStream(target);

reader.on('close', () => {
    console.log('reader closed');
});

writer.on('close', () => {
    console.log('writer closed');
});

reader.on('data', () => {
  console.log('receive data')
})

reader.on('readable', () => {
  // DO NOT `reader.read()`
})

Will print:

receive data
reader closed

However, change the order of binding these two events will result in different behaviors:

const fs = require('fs')
const { resolve } = require('path')

const source = resolve(__dirname, 'index.js')
const target = resolve(__dirname, 'copy.js')

const reader = fs.createReadStream(source)
const writer = fs.createWriteStream(target);

reader.on('close', () => {
    console.log('reader closed');
});

writer.on('close', () => {
    console.log('writer closed');
});

reader.on('readable', () => {
  // DO NOT `reader.read()`
})

reader.on('data', () => {
  console.log('receive data')
})

Will print nothing and exit.

Also, replace reader.on('data') with reader.pipe(writer) will cause similar results.

[mcollina]

@oyyd yes it does. Unfortunately the documentation for #18994 was not clear enough. This resolves an edge case for that PR and it improves the docs.

I've also replied in the issue you linked.

[oyyd]

I see, thanks.

[mcollina]

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

[mafintosh]

LGTM, nice fix

[mcollina]

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

[mcollina]

@nodejs/build I can't get centos7-64-gcc6 to pass, see https://ci.nodejs.org/job/node-test-commit-linux/20945/nodes=centos7-64-gcc6/console.

This looks like an infra issue, see nodejs/build#1468.

[Trott]

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16665/

[Trott]

Resume Build resulted in a green CI run.

Landed in 98cf84f.