-
Notifications
You must be signed in to change notification settings - Fork 170
[AsyncThrowingChannel] make the fail
terminal event non async
#164
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
[AsyncThrowingChannel] make the fail
terminal event non async
#164
Conversation
it might be worthwhile to investigate how this interacts with #167 |
Hi. I think they collide. This PR also addresses the issue where the failure is lost. I encountered the problem while testing the “non async” version of the fail event. Our implementation of the “Termination” enum is very similar except i have decided to remove the “terminal” variable and to add a new “terminated” case to the Emission enum. As “finish” or “fail” are both terminal events, I think it makes sense to integrate this concept directly to the Emission state. This way we can benefit from the exclusive values of the enum and we can’t be in both “pending” and “terminated” state at the same time for instance. What do you think ? @phausler |
I think if we are removing the back pressure on terminal events then your approach might be better. The one gotcha that I did run across is that once you consume the failure it needs to transition back to just a normal finished state; e.g. the case of iterating past a failure. |
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.
Modulo the one issue with terminal transitions after it being emitted this looks great.
7db6c3d
to
4831b0f
Compare
thanks for the review @phausler. I've pushed a new version. |
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.
looks solid to me!
This PR is the sister of #152 and is linked to that issue #155.
Long story short: terminal events (finish/fail) should not imply a back pressure management since no further elements can be sent.
When fail is called:
next()
will throw the errorsend(_:)
will immediately resume