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: finished depends on internal API #28813

Closed
1 task
ronag opened this issue Jul 23, 2019 · 4 comments
Closed
1 task

stream: finished depends on internal API #28813

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

Comments

@ronag
Copy link
Member

ronag commented Jul 23, 2019

Currently the implementation of finished/eos depends on internal streams API's which means that there is currently no proper way to implement "streamlike" objects that are compatible.

We've improved the situation with writableFinished but there are still a lot of internal properties that need to be accessed for proper function. Currently e.g. OutgoingMessage has some limitations in regards to this (e.g. #28748 won't work).

I think what needs to be added to the streams API is:

  • readableEnded

Also, I think readableEnded/writableFinished should be changed so that it becomes true after 'end'/'finish' has been emitted (not like it is today where it is set before).

@ronag ronag changed the title stream.finished depends on internal API stream: finished depends on internal API Jul 23, 2019
@ronag
Copy link
Member Author

ronag commented Jul 23, 2019

@mcollina

@ronag ronag mentioned this issue Jul 23, 2019
4 tasks
@mcollina
Copy link
Member

+1 to readableEnded!

Also, I think readableEnded/writableFinished should be changed so that it becomes true after 'end'/'finish' has been emitted (not like it is today where it is set before).

I'm -1 to this change unless it's highly motivated. These things are always going to cause trouble.

@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Jul 25, 2019
Trott pushed a commit that referenced this issue Aug 20, 2019
Adds a readableEnded property and improved finished compat with possible
stream-like objects.

PR-URL: #28814
Refs: #28813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Aug 20, 2019
Adds a readableEnded property and improved finished compat with possible
stream-like objects.

PR-URL: #28814
Refs: #28813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag
Copy link
Member Author

ronag commented Aug 24, 2019

This has been sorted

@ronag ronag closed this as completed Aug 24, 2019
@kanongil
Copy link
Contributor

Also, I think readableEnded/writableFinished should be changed so that it becomes true after 'end'/'finish' has been emitted (not like it is today where it is set before).

I'm -1 to this change unless it's highly motivated. These things are always going to cause trouble.

Yup, this is a horrible change that is causing trouble and means that my code will still have to rely on _readableState.ended (or some annoying workaround) to work.

FYI, I use it to avoid calling stream.destroy(err) when the stream has already already ended through stream.push(null).

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

4 participants