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: throw instead of destroy? #33192

Closed
rexagod opened this issue May 1, 2020 · 5 comments
Closed

stream: throw instead of destroy? #33192

rexagod opened this issue May 1, 2020 · 5 comments

Comments

@rexagod
Copy link
Member

rexagod commented May 1, 2020

I've come across multiple instances (in http2, for example) where due to a condition not being fulfilled, an error was thrown rather than destroying the stream itself (which would emit an error event and hence would be easier to work with).

I wanted to know if there's any reason behind not destroying the stream, or if this needs to be fixed?

@rexagod rexagod changed the title stream: throws instead of destroy? stream: throw instead of destroy? May 1, 2020
@ronag
Copy link
Member

ronag commented May 1, 2020

I wanted to know if there's any reason behind not destroying the stream, or if this needs to be fixed?

This has been discussed a few times in the past. Most recently in #31818.

Some refs:

#31831 (comment)
#31831
#31818

@ronag
Copy link
Member

ronag commented May 1, 2020

It's a case of logic (throw) vs runtime (destroy) errors.

@rexagod
Copy link
Member Author

rexagod commented May 2, 2020

Thank you for the quick response, ronag!

Quoting @mscdex from #31818 (review),

Not throwing errors goes against behavior found throughout node.

Throwing should be used when there is a user error of one kind or another that can be immediately detected: when the stream has already ended (something that the user could check easily first), if an argument of the wrong type is passed, etc.

I'll work on a PR soon to ensure that this logic is implemented in streams (and HTTP[s]\1\2).

@ronag
Copy link
Member

ronag commented May 3, 2020

I'll work on a PR soon to ensure that this logic is implemented in streams (and HTTP[s]\1\2).

Please note that this is not currently the consensus. Throwing is not always preferred. Please read the rest of the discussion.

@BridgeAR
Copy link
Member

I am closing this as there does not seem anything in particular actionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants