-
-
Notifications
You must be signed in to change notification settings - Fork 420
Infinite steams cookbook recipe #1849
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
Infinite steams cookbook recipe #1849
Conversation
|
Looks like a new release of crypton-x509-store contains a breaking change, preventing the build. Unrelated to this PR |
|
@LaurentRDC The package's metadata seems to have been amended on Hackage. :) |
5cc3381 to
0772f4c
Compare
0772f4c to
6643298
Compare
|
@LaurentRDC Thanks for the PR. I left some comments for clarification. :) |
6643298 to
6945b21
Compare
|
Thanks for your review! While addressing your comments, I have also refactored the tricky logic that interleaves integers with |
| go (SourceT.Error err) = error err | ||
| go (SourceT.Stop) = error "Unexpected stream end" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing a "graceful" handling of stream completion in these two cases. This is purely due to my own ignorance as I don't do stream processing in my day-to-day job.
Would you advise that they both (Error & Stop) be handled in the same way (a log entry with a callstack, for instance), or should there be different "finaliser" code in one or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cookbook is really concerned with infinite streams. As such, there should not be a need for graceful shutdown, as in, this part of the code is unreachable.
I have cleaned up cases, by having separate finalizers: a stream Error results in an exception being thrown, and a Stop results in pure (). Let me know what you think.
6945b21 to
44aab7a
Compare
|
@LaurentRDC Wonderful. One last question and I'll merge: What is the reasoning behind the use of |
There's no deep reason. I personally think of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, let's go!
|
Thanks for reviewing @tchoutri ! |
|
@LaurentRDC It was also the opportunity to learn more. :) Hope to see more cookbooks from you! :) |
|
@LaurentRDC I came across this page that explains more why |
|
Very interesting! Thanks for sharing! |
This is a cookbook recipe to serve infinite HTTP streams. There are a lot of subtleties in providing an HTTP infinite stream because data must be sent to the client on a regular basis.