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.

Sign up for GitHub

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

Jump to bottom

Add bi-directional cancellation for Task.await and Task.asDeferred #2528

Closed
wants to merge 8 commits into from
Closed

Add bi-directional cancellation for Task.await and Task.asDeferred #2528

wants to merge 8 commits into from

Conversation

alexvanyo
Copy link
Contributor

@alexvanyo alexvanyo commented Feb 5, 2021 •
edited

This PR includes my proposed addition to the play-services integration to allow bi-directional cancellation for Task.await and Task.asDeferred, which in particular provides the hook to cancel a task via a CancellationToken (which fixes #2527)

Both Task.asDeferred and Task.await now each have a new overload that takes a passed in CancellationTokenSource. If the Deferred/await is cancelled, or if the receiving Task is cancelled, then the CancellationTokenSource will also be cancelled. In particular, this means that if the Task will be cancelled when the CancellationTokenSource is cancelled, cancelling the Deferred/await will cancel the underlying Task.

The existing implementations of Task.asDeferred and Task.await call into the new implementations, with a newly constructed CancellationTokenSource that will result in a no-op upon being cancelled.

@qwwdfsad qwwdfsad self-requested a review February 18, 2021 17:33
qwwdfsad
qwwdfsad requested changes Jun 7, 2021
View reviewed changes
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please don't forget to run ./gradlew apiDump in order to updated our binary compatibility data (you can read more here).

If you are not ready to walk this extra mile, that's completely okay, just notify us

integration/kotlinx-coroutines-play-services/src/Tasks.kt Outdated Show resolved Hide resolved
@alexvanyo
Copy link
Contributor Author

Thanks for the review @qwwdfsad , this should be ready for another look!

The apiDump should also be good to go as well!

@alexvanyo alexvanyo requested a review from qwwdfsad June 7, 2021 18:51
@alexvanyo alexvanyo changed the title Add await for task created with cancellation token Add bi-directional cancellation for Task.await and Task.asDeferred Jun 8, 2021
alexvanyo
alexvanyo commented Jun 8, 2021
View reviewed changes
integration/kotlinx-coroutines-play-services/src/Tasks.kt
* [CancellationException].
* If the [cancellationTokenSource] is cancelled, then this function will throw a [CancellationException].
*/
@ExperimentalCoroutinesApi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these could be combined to have cancellationTokenSource be a default argument, but then I couldn't apply @ExperimentalCoroutinesApi to just the new overload.

@alexvanyo alexvanyo force-pushed the await-constructed-task branch 2 times, most recently from 3988d1e to d66ae38 Compare June 8, 2021 15:42
@alexvanyo alexvanyo requested a review from qwwdfsad June 8, 2021 15:48
@alexvanyo
Copy link
Contributor Author

Thank you for the review and insights!

I switched the implementation to an overload of Task.await that takes a CancellationTokenSource argument, and also added a mirroring implementation for Task.asDeferred to also support bi-directional cancellation there.

I'm curious to hear your thoughts on the handling for when the passed-in CancellationTokenSource isn't directly related to the Task, since we can't guarantee that will be the case.

@alexvanyo alexvanyo requested a review from qwwdfsad June 16, 2021 21:48
qwwdfsad
qwwdfsad reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Please take care of the test and I'll handle the rest minors and will ensure it makes it into the next release

integration/kotlinx-coroutines-play-services/test/TaskTest.kt Show resolved Hide resolved
@alexvanyo alexvanyo requested a review from qwwdfsad June 22, 2021 01:27
@alexvanyo
Copy link
Contributor Author

Just saw that the tests failed again, I went ahead and removed a similar racing assertion from the Deferred tests as well in d9d886b.

@qwwdfsad qwwdfsad mentioned this pull request Jun 23, 2021
Merged
qwwdfsad
qwwdfsad approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks!

I've opened #2786 with minor edits, will merge it and it's going to be the part of 1.5.1.

@qwwdfsad qwwdfsad closed this Jun 23, 2021
@alexvanyo alexvanyo deleted the await-constructed-task branch June 23, 2021 14:59
@alexvanyo
Copy link
Contributor Author

Awesome, thank you @qwwdfsad for your help and your time to review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwwdfsad qwwdfsad qwwdfsad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexvanyo @qwwdfsad