Skip to content

http,stream: remove usage of _readableState #20051

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

Closed

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Apr 15, 2018

Remove the usage of the restricted _readableState property and use the
readableFlowing property instead.

Refs: #445

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This should work but doesn't. According to the original code:

  // if the user never called req.read(), and didn't pipe() or
  // .resume() or .on('data'), then we call req._dump() so that the
  // bytes will be pulled off the wire.
  if (!req._readableState.resumeScheduled)
    req._dump();

Technically, it should be called whenever the stream isn't accessed.

According to the docs of Readable at https://nodejs.org/api/stream.html#stream_readable_streams:

  1. req must be a Readable stream.
  2. readable.readableFlowing = null when the above condition is true (Ref: https://nodejs.org/api/stream.html#stream_three_states)

Therefore, substituting the above condition by if (req.readableFlowing === null) must work.

But it doesn't.

@mcollina @mafintosh @nodejs/streams any idea why?

Remove the usage of the restricted _readableState property and use the
readableFlowing property instead.

Refs: nodejs#445
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 15, 2018
@mcollina
Copy link
Member

Therefore, substituting the above condition by if (req.readableFlowing === null) must work.
But it doesn't.

What do you mean by "it does not work"?

Note that resumeScheduled means something different. It means that there will be a resume() in the next tick. Then it will set readableFlowing accordingly. readableFlowing is null if there is no resume() going.

@ryzokuken
Copy link
Contributor Author

@mcollina I was definitely mislead to believe they behaved in similar ways by the docs. Anyway, in the case, do I have to expose a non-private resumeScheduled property then?

@mcollina
Copy link
Member

I'm not necessarily certain that resumeScheduled is actually the correct check there. I would try to implemented something like readableStarted, with the meaning of "this stream is going to be consumed". it needs to check also for readableListening.

Exposing a new property would be fine, but this seems rather peculiar, and I truly do not know what is the right path here.

@ryzokuken
Copy link
Contributor Author

@mcollina I'd definitely love to help out as much as I can, but considering the complexity of this problem and my relative lack of knowledge about how streams work internally, that might not be much.

Should I close this?

@mcollina
Copy link
Member

Yes, please. Also this area is being worked by @apapirovski in #20088.

@mcollina mcollina closed this Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants