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

AsyncChannel fixes potential crashes when no more awaiting #143

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

twittemb
Copy link
Contributor

This PR fixes potential crashes where the state is "awaiting" while there are no more awaiting clients.
The state is now set to .idle to reflect the reality.

This PR is linked to the issue #142.

The fix prevents the removeFirst() to be executed by preemptively setting the state to a correct value. It think the removeFirst() statements should be used in a more secure fashion though. It could be the subject of another PR.

It is hard to produce unit tests that ensure the crashes are fixed since we must set the AsyncChannel in an awaiting state. Ideas are welcome !

@twittemb twittemb changed the title asyncChannel: fixes potential crashes when no more awaiting AsyncChannel fixes potential crashes when no more awaiting Apr 11, 2022
@phausler
Copy link
Member

is there a stochastic test that we could add? e.g. something that if we run it 100/1000x times etc that it is likely to crash?

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.

These changes look correct to me, perhaps we can also use the validation diagrams to build something to isolate this deterministically.

@twittemb
Copy link
Contributor Author

twittemb commented Apr 11, 2022

Regarding the stochastic test, it seems hard because the last instruction of the test would be to send an element in the Channel to ensure it does not crash ... but if there is no crash then this last instruction will be awaited for ever, then the test never finishes.

func test_cancel_when_only_one_iterator_while_waiting() async throws {
  for _ in (0...100) {
    let channel = AsyncChannel<Int>()

    let task = Task {
      for await _ in channel {}
    }

    task.cancel()

   await channel.send(1) // -> never resume
  }
}

About the validation diagram, I'm not super aware of how it works but I can't see a way to interact with a Channel in the DSL. What do you have in mind ?

One way would be to test the internal state machine of the AsyncChannel but we would need a spy callback in the next() function to be able to trigger the task's cancellation in the unit test using an expectation. Doing so we only test the internal state machine though, we don't test the behaviour of the Channel as a whole.

This commit fixes crashes where the state is "awaiting" with no more awaiting clients.
The state is now set to .idle to reflect the reality.
@twittemb
Copy link
Contributor Author

@phausler do you have other ideas ? I've tried to give it another thought today and I cannot figure out a way to test that it does not crash without having spy callbacks and internal state exposed :-(

@phausler
Copy link
Member

sadly I have not had a lot of time to ruminate on an alternative to test this. To me it seems right, so I am going to go ahead and merge it with an issue to test it later.

@phausler phausler merged commit 047ab6a into apple:main Apr 15, 2022
@twittemb twittemb deleted the fix/asyncchannel-awaiting branch May 4, 2022 20:17
manmal pushed a commit to Diabetes-Cockpit/swift-async-algorithms that referenced this pull request Jul 16, 2022
This commit fixes crashes where the state is "awaiting" with no more awaiting clients.
The state is now set to .idle to reflect the reality.
# Conflicts:
#	Sources/AsyncAlgorithms/AsyncChannel.swift
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.

None yet

2 participants