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

Fix async channel finishes all #152

Merged
merged 5 commits into from
May 4, 2022

Conversation

twittemb
Copy link
Contributor

@twittemb twittemb commented May 1, 2022

This PR aims to fix/improve the "finish" strategy for AsyncChannel/AsyncThrowingChannel.

From my understanding there are two possible scenarios when sending a finish:

- 1: There are no awaiting clients. There are potential pending "send" operations. We send finish 1 or several times.
- 2: There are no pending operations. There are several awaiting clients. We send finish 1 or several times.

The current behaviour is:

- 1: The state is set to terminal. Finish is added as a pending operation (other future finishes or values will be discarded). The future clients will consume the pending operations until they reach the finish operation. Once a client has consumed this finish operation, the next potential awaiting clients will be suspended for ever.
- 2: The state is set to terminal. The first awaiting operation is resumed with the nil value. The potential other awaiting operations won't be resumed. There is also infinite suspensions for them.

The behaviour with this PR is:

- 1 and 2: All pending and awaiting operations are resumed with nil (other furure finishes or values will be discarded). The potential futur clients will be instantly resumed with nil.

Some considerations that might trigger questions:

  • the finish function is no more async since it does not involve the creation of a continuation.
  • If there are several pending operations when finish is called, they will be discarded, meaning that if we have several Tasks that send values in a Channel and one of them call finish then some potential suspended send in other tasks will be resumed instantly and their value will never be emitted.
  • Unit testing the scenario where finish is called while several clients are already awaiting is difficult because there is no way to wait for the next function to be called before calling finish. Ideas are welcome.

This PR addresses the issue described here -> #136

@phausler
Copy link
Member

phausler commented May 3, 2022

Couple of tasks to wrap this up:

  • Update the documentation for finish to reflect it's non-awaiting characteristics.
  • Update the guide to reflect the historical designs here and the new non-async-ness of the finish function

@twittemb
Copy link
Contributor Author

twittemb commented May 3, 2022

Couple of tasks to wrap this up:

  • Update the documentation for finish to reflect it's non-awaiting characteristics.
  • Update the guide to reflect the historical designs here and the new non-async-ness of the finish function

Definitely. I will do that next week as I don’t have my mac with me this week.

@parkera parkera added the v1.0 Work leading up first API stable version label May 4, 2022
@parkera
Copy link
Member

parkera commented May 4, 2022

@swift-ci test

@twittemb twittemb force-pushed the fix/asyncChannel-finishes-all branch from b2fbfef to 07f282d Compare May 4, 2022 19:17
@phausler
Copy link
Member

phausler commented May 4, 2022

the macOS tests fell over because it is executing the wrong test system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.0 Work leading up first API stable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants