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.resume() doesn't make the stream flowing after removing readable listener #24281

Closed
snakamura opened this issue Nov 10, 2018 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@snakamura
Copy link

snakamura commented Nov 10, 2018

  • Version: v10.13.0
  • Platform: Darwin planata.local 17.7.0 Darwin Kernel Version 17.7.0: Wed Oct 10 23:06:14 PDT 2018; root:xnu-4570.71.13~1/RELEASE_X86_64 x86_64 i386 MacBookPro14,2 Darwin
  • Subsystem: stream

With v10.13.0, stream.resume() doesn't make the stream flowing after removing readable listener. Here is a code to demonstrate the problem.

'use strict';

const fs = require('fs');

const s = fs.createReadStream('s.js');

const readableListener = () => console.log('readable');
s.on('readable', readableListener);
s.on('end', () => console.log('end'));
s.removeListener('readable', readableListener);
s.resume();

Note that you need to name this script s.js to run this script without any changes.

With v10.13.0, this prints nothing. With v8.12.0, this prints end. This seems to be related to #18994, #21696 and #22209, but I'm not sure if this is an intentional change.

Note that a stream starts flowing when I add data listener instead of calling resume.

@targos
Copy link
Member

targos commented Nov 10, 2018

@nodejs/streams

@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Nov 10, 2018
@shobhitchittora
Copy link
Contributor

Hey @snakamura! As per the docs -

The readable.resume() method has no effect if there is a 'readable' event listener.

So if you remove your readable listener, you get end printed. Does that help!

@snakamura
Copy link
Author

@shobhitchittora That's what I expected, and v10.13.0 didn't work like that as you can see in my code.

@mcollina mcollina added the confirmed-bug Issues with confirmed bugs. label Nov 14, 2018
@mcollina
Copy link
Member

Confirmed, we need to have a look in more detail.

@mcollina
Copy link
Member

Check out #24366.

@mcollina
Copy link
Member

Fixed in 69cc58d.

mcollina added a commit that referenced this issue Nov 21, 2018
Fixes: #24281

PR-URL: #24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
targos pushed a commit that referenced this issue Nov 21, 2018
Fixes: #24281

PR-URL: #24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@snakamura
Copy link
Author

Thank you!

rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #24281

PR-URL: #24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@snakamura
Copy link
Author

@nodejs/backporters Sorry to bother you, but are there chances that this fix will be backported to 10.x LTS?

codebytere pushed a commit that referenced this issue Jan 13, 2019
Fixes: #24281

PR-URL: #24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
Fixes: nodejs#24281

PR-URL: nodejs#24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
codebytere pushed a commit that referenced this issue Jan 29, 2019
Fixes: #24281

PR-URL: #24366
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants