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: read after destroy? #29477

Closed
ronag opened this issue Sep 6, 2019 · 2 comments
Closed

stream: read after destroy? #29477

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

Comments

@ronag
Copy link
Member

ronag commented Sep 6, 2019

Reading the Readable.read implementation it is a bit unclear to me what happens and what should happen when this method is called after destroy().

Should it be an error (e.g. READ_AFTER_DESTROY) or a noop?

@addaleax
Copy link
Member

addaleax commented Sep 7, 2019

I think both behaviours are okay, so I’d go with the variant that is less breaking, i.e. making it a noop.

@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Sep 7, 2019
@ronag
Copy link
Member Author

ronag commented Sep 8, 2019

@addaleax: I've done a quick fix for the most problematic parts #29491.

However, there seems to be an assumption that readable streams will still work with buffered data after destroy(), i.e. 'end' (and 'finish' for that matter) is expected to be emitted after destroy() in several tests.

I started on a "proper" fix but it quickly hit several tests #29485.

Thoughts? @mcollina

@ronag ronag closed this as completed Oct 6, 2019
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

Successfully merging a pull request may close this issue.

2 participants