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

eos and stream type detection #29395

Closed
ronag opened this issue Sep 1, 2019 · 4 comments
Closed

eos and stream type detection #29395

ronag opened this issue Sep 1, 2019 · 4 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 1, 2019

Looking at https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js#L31

It looks like "eos" uses the writable and readable properties to detect whether a stream is Readablelike or Writablelike. However, according my current incomplete understanding of these properties (#29377), they cannot/should not be used for this purpose, i.e. a Readable stream could in theory have a readable = false which later becomes a readable = true.

Should we maybe change these lines to something like e.g:

const isReadable = (
  typeof stream.readable === 'boolean' || 
  stream._readableState || 
  typeof stream.readableEnded === 'boolean'
);
let readable = opts.readable || (opts.readable !== false && isReadable);
@ronag
Copy link
Member Author

ronag commented Sep 1, 2019

ping @mscdex @mcollina

@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Sep 1, 2019
@mcollina
Copy link
Member

mcollina commented Sep 2, 2019

You are correct. You cannot desume if something is going to be readable/writable in the future by looking at those properties, only if it could be readable/writable now. I think we should improve eos in this regard.

@ronag
Copy link
Member Author

ronag commented Sep 2, 2019

I think we can assume that is something a readable and is not destroyed it will become readable at some point.

I think what we primarily need to improve is how we detect whether something is Writable or Readable, agree?

ronag added a commit to nxtedition/node that referenced this issue Sep 3, 2019
The value of stream.readable and stream.writable should not
be used to detect whether a stream is Writable or Readable.

Refs: nodejs#29395
Trott pushed a commit that referenced this issue Sep 22, 2019
The value of stream.readable and stream.writable should not
be used to detect whether a stream is Writable or Readable.

Refs: #29395
PR-URL: #29409
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag
Copy link
Member Author

ronag commented Oct 6, 2019

This has been fixed.

@ronag ronag closed this as completed Oct 6, 2019
targos pushed a commit that referenced this issue Jan 14, 2020
The value of stream.readable and stream.writable should not
be used to detect whether a stream is Writable or Readable.

Refs: #29395
PR-URL: #29409
Backport-PR-URL: #31345
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
The value of stream.readable and stream.writable should not
be used to detect whether a stream is Writable or Readable.

Refs: #29395
PR-URL: #29409
Backport-PR-URL: #31345
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

3 participants