Skip to content

[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

Merged
merged 4 commits into from
Jun 27, 2022

Conversation

twittemb
Copy link
Contributor

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:

  • all pending and awaiting operations should be immediately resumed (in order to avoid potential infinite suspensions)
  • further calls to next() will throw the error
  • further calls to send(_:) will immediately resume

@phausler
Copy link
Member

it might be worthwhile to investigate how this interacts with #167

@twittemb
Copy link
Contributor Author

twittemb commented Jun 16, 2022

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

@phausler
Copy link
Member

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.

Copy link
Member

@phausler phausler left a 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.

@twittemb twittemb force-pushed the fix/asyncThrowingChannel-failure branch from 7db6c3d to 4831b0f Compare June 16, 2022 17:33
@twittemb
Copy link
Contributor Author

Modulo the one issue with terminal transitions after it being emitted this looks great.

thanks for the review @phausler. I've pushed a new version.

Copy link
Member

@phausler phausler left a 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!

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.

2 participants