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

Jump to bottom

Deprecate Readable.readableFlowing setter #39495

Open
ronag opened this issue Jul 23, 2021 · 7 comments · May be fixed by #39644
Open

Deprecate Readable.readableFlowing setter #39495

ronag opened this issue Jul 23, 2021 · 7 comments · May be fixed by #39644
Labels
good first issue stream

Comments

@ronag
Copy link
Member

@ronag ronag commented Jul 23, 2021

How this works and what it does is pretty undefined and I don't think users should be using this setter.
@ronag ronag added the stream label Jul 23, 2021
@ronag
Copy link
Member Author

@ronag ronag commented Jul 23, 2021

@nodejs/streams

@ronag ronag changed the title Deprecate Readable.readableFlowing setter Deprecate Readable.readableFlowing setter Jul 23, 2021
@ronag ronag changed the title Deprecate Readable.readableFlowing setter Deprecate Readable.readableFlowing setter Jul 23, 2021
@mcollina
Copy link
Member

@mcollina mcollina commented Aug 3, 2021

I'm +1. I think I added it because we needed it somewhere in core.

@ronag
Copy link
Member Author

@ronag ronag commented Aug 3, 2021

There might be one case where it makes sense to have it: https://github.com/nodejs/undici/blob/main/lib/client.js#L723-L733

Not sure how we could work around that. Maybe change undici so it uses read() + 'readable'.

@Mesteery
Copy link
Member

@Mesteery Mesteery commented Aug 3, 2021

It is also used in :

node/lib/_http_client.js

Line 523 in 533cafc

socket.readableFlowing = null;

node/lib/_http_server.js

Line 719 in 533cafc

socket.readableFlowing = null;

node/lib/net.js

Line 384 in 533cafc

this.readableFlowing = false;

node/lib/internal/child_process.js

Line 438 in 533cafc

stream._stdio.readableFlowing = false;

@ronag
Copy link
Member Author

@ronag ronag commented Aug 3, 2021

It's probably easier to keep using it there than fixing it. I'm more concerned with the public api to our users.

ronag added a commit to nodejs/undici that referenced this issue Aug 3, 2021
@ronag
fix: avoid access internal socket state
2083e12 
Refs: nodejs/node#39495
@ronag ronag mentioned this issue Aug 3, 2021
Merged
ronag added a commit to nodejs/undici that referenced this issue Aug 3, 2021
@ronag
fix: avoid access internal socket state
873550a 
Refs: nodejs/node#39495
Mesteery added a commit to Mesteery/node that referenced this issue Aug 3, 2021
@Mesteery
stream: runtime deprecate readableFlowing setter
e08eb77 
Fixes: nodejs#39495
Mesteery added a commit to Mesteery/node that referenced this issue Aug 3, 2021
@Mesteery
stream: deprecate readableFlowing setter
e782e5b 
Fixes: nodejs#39495
@Mesteery Mesteery linked a pull request that will close this issue Aug 3, 2021
Open
ronag added a commit to nodejs/undici that referenced this issue Aug 3, 2021
@ronag
fix: avoid access internal socket state
790a3b9 
Refs: nodejs/node#39495
ronag added a commit to nodejs/undici that referenced this issue Aug 3, 2021
@ronag
fix: avoid access internal socket state (#920)
1d560c4 
Refs: nodejs/node#39495
@ktfth
Copy link

@ktfth ktfth commented Aug 6, 2021

How about the evolution of this implementation?

@ktfth
Copy link

@ktfth ktfth commented Aug 6, 2021

Have something who I can help? Some guidance could be great

Mesteery added a commit to Mesteery/node that referenced this issue Aug 11, 2021
@Mesteery
stream: deprecate readableFlowing setter
7ac4f48 
Fixes: nodejs#39495
jodevsa pushed a commit to jodevsa/undici that referenced this issue Nov 1, 2021
@ronag @subhialhasan
fix: avoid access internal socket state (nodejs#920)
4d16d01 
Refs: nodejs/node#39495
jodevsa pushed a commit to jodevsa/undici that referenced this issue Dec 4, 2021
@ronag @subhialhasan
fix: avoid access internal socket state (nodejs#920)
d56eb24 
Refs: nodejs/node#39495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue stream
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

stream: deprecate readableFlowing setter
4 participants
@mcollina @ronag @ktfth @Mesteery