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

Close stream only on sync-exception #191

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

kamoii
Copy link
Contributor

@kamoii kamoii commented Aug 14, 2019

I had the same problem as #182 . This PR solves it.

When an exception occurs while reading/sending againt Stream, the stream was closed. This PR changes only to close on sync exceptions. Async exceptions will remain the stream opened.

While this changes solves my case, I'm not sure this behaviour is acceptable generally.

@thomasjm
Copy link

thomasjm commented Oct 5, 2019

Oh @kamoii I didn't see your PR when I opened #193 yesterday. FYI my PR restricts the set of caught exceptions even further to just the WebSocket exceptions, which also seems to work.

@jaspervdj
Copy link
Owner

I think both approaches make sense -- the exact set of exceptions that would need to be caught also includes IOException and exceptions generated from "middleware" libraries like ZLib, so I'm inclined to go with this PR -- any sync exception during receive is probably a real problem.

Does that seem fine with you @thomasjm? Or am I forgetting any weird cases in which this would cause issues?

@thomasjm
Copy link

Hmm @jaspervdj , I don't know what exceptions might come from internal libraries. I think in general exceptions should be well-understood and libraries should document which exceptions they might throw. For that reason explicitly listing the exceptions we want to catch seems better to me. But I can't think of any cases off the top of my head.

@jaspervdj jaspervdj merged commit c36ae3c into jaspervdj:master Oct 11, 2019
@jaspervdj
Copy link
Owner

Okay, I'll go with this PR for now.

For what it's worth, I completely agree that languages in general should prefer checked exceptions, unfortunately Haskell isn't a perfect language (yet) and there's no easy way to determine what exceptions can be thrown from send & receive, across different network versions and especially since the user can add their own send & receive...

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

Successfully merging this pull request may close these issues.

3 participants