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

Behavior of readablestreams changed in v10 #20520

Closed
tmcw opened this issue May 4, 2018 · 10 comments
Closed

Behavior of readablestreams changed in v10 #20520

tmcw opened this issue May 4, 2018 · 10 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@tmcw
Copy link

tmcw commented May 4, 2018

  • Version: v10.0.0
  • Platform:Darwin tmcw.local 17.5.0 Darwin Kernel Version 17.5.0: Fri Apr 13 19:32:32 PDT 2018; root:xnu-4570.51.2~1/RELEASE_X86_64 x86_64
  • Subsystem: stream

Hi Maintainers!

This may be the same underlying issue as #20503 - I'm not confident enough in the node core space enough to tell.

This problem was first noticed in cp-file, a dependency of cpy, which is a dependency of one of my projects. A very distilled test case is:

const fs = require('fs');
const read = fs.createReadStream(__filename);

read.on('readable', () => {
    read.on('data', chunk => {
        console.log('data');
    });
});

This is a program that reads itself, by creating a read stream, waiting for it to be readable, and then assigning a data event listener. It prints data on all node versions < 10, but prints nothing on the Node v10.0.0 release.

In situ, cp-file tries to ensure that a stream is readable before creating the destination directory and then calling .pipe to write the file to disk. As far as I can tell, Node v10 changes the behavior of readable streams such that data events are not emitted if you bind the data event listener after readable is emitted.

@vsemozhetbyt vsemozhetbyt added the stream Issues and PRs related to the stream subsystem. label May 4, 2018
@lpinca
Copy link
Member

lpinca commented May 6, 2018

I didn't check but I think behavior changed with #18994.
cc: @nodejs/streams

@mcollina
Copy link
Member

mcollina commented May 6, 2018

The provided example is not a good way of using streams, because it will add a new 'data'  handler for each emitted chunk (even on Node < 10). In the cp-file it will call resolve every time there is a new chunk.

The solution (also to the cp-file case) is to use once() instead of on(). See sindresorhus/copy-file#26.

I think this can be closed.

@lpinca lpinca closed this as completed May 6, 2018
@schnittstabil
Copy link

schnittstabil commented May 6, 2018

it will call resolve every time there is a new chunk.

That is not a problem, because of the semantics of Promises. You may call resolve as often as you want, only the first one matters:

new Promise((resolve, reject) => {
    resolve('resolve: ' + 1);
    resolve('resolve: ' + 2);
    resolve('resolve: ' + 3);

    reject('reject: ' + 1);
    reject('reject: ' + 2);
    reject('reject: ' + 3);
})
.then(console.log)
.catch(console.error);

// outputs on node v9 and v10
resolve: 1

Please also note, that your suggestion (using once for data events) does not solve the problem in the example of @tmcw:

const fs = require('fs');
const read = fs.createReadStream(__filename);

read.on('readable', () => {
    read.once('data', chunk => {
        console.log('data');
    });
});

// nodejs v9
//=> data

// nodejs v10
//=> (no output)

@mcollina Interestingly, your suggestion to use .once instead of .on for readable events seems to solve the issues for cp-file and the example above:

read.once('readable', () => {
    read.on('data', chunk => {
        console.log('data');
    });
});

// nodejs v9 and v10
//=> data

Thus, it seems .on('readable') and .once('readable') behave differently.

@lpinca
Copy link
Member

lpinca commented May 6, 2018

That is not a problem, because of the semantics of Promises. You may call resolve as often as you want, only the first one matters

Yes but it's useless overhead.

Please also note, that your suggestion (using once for data events) does not solve the problem in the example of @tmcw:

The suggestion was to use once for 'readable', not for 'data'.

The behavior change introduced in #18994 is semver-major but it seems to work as intended.

@schnittstabil
Copy link

schnittstabil commented May 6, 2018

The behavior change introduces in #1899 is semver-major but it seems to work as intended.

That PR is 3 years old and rather long. Moreover the discussion does not contain the words read, readable or stream – Where do I have to look?

I'm not sure what your point is. It is unexpected, if .on('readable') and .once('readable') behave differently, isn't it? (except for the differences mentioned at Handling events only once of course 😉 )

@BridgeAR
Copy link
Member

BridgeAR commented May 6, 2018

It was a typo when referencing the issue. The "4" was missing at the end. I fixed that by editing the reference.

@schnittstabil
Copy link

@BridgeAR Thank you, for clarifying. But, please do not edit my comments in a way, so the discussion does not make sense for readers anymore.

@lpinca
Copy link
Member

lpinca commented May 6, 2018

@schnittstabil my point is that cf5f986 seems to work as expected, specifically:

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.

@schnittstabil
Copy link

Ok, but both .on('readable', …) and .once('readable', …) create listeners for the
'readable' event
, don't they? – I believe, I still don't understand 😕

@lpinca
Copy link
Member

lpinca commented May 6, 2018

The difference is that if there is at least one listener for the 'readable' event, on('data', fn) will not resume the stream. once() in the above example makes sure that there are no listeners when on('data', fn) is called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants