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

Repair some corner cases in cancellation propagation between coroutines and listenable futures #2222

Merged
merged 3 commits into from Nov 26, 2020

Conversation

vadimsemenov
Copy link
Contributor

Implement bidirectional cancellation for future coroutine builder.

This also:

  • Refactors JobListenableFuture infrastructure so it can be reused in CoroutineScope.future and Deferred.asListenableFuture;
  • Provides more descriptive toString implementation for the returned Future;
  • Fixes stack traces in thrown exception, so it includes call to get() that triggered the exception to be thrown;
  • Hides ListenableFuture.asDeferred return type, so it can't be casted to CompletableDeferred;
  • Adds more tests to cover fixed corner cases;
  • Improves documentation;
  • Suppresses annoying warnings in tests.

…es and listenable futures

Implement bidirectional cancellation for `future` coroutine builder.

This also:

* Refactors JobListenableFuture infrastructure so it can be reused in CoroutineScope.future and Deferred.asListenableFuture;
* Provides more descriptive `toString` implementation for the returned Future;
* Fixes stack traces in thrown exception, so it includes call to get() that triggered the exception to be thrown;
* Hides ListenableFuture.asDeferred return type, so it can't be casted to CompletableDeferred;
* Adds more tests to cover fixed corner cases;
* Improves documentation;
* Suppresses annoying warnings in tests.
@vadimsemenov
Copy link
Contributor Author

Fixes #1442

@vadimsemenov
Copy link
Contributor Author

Ping?

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 job! A decent number of comments, every change and corner case is tested, really valuable contribution!

Let's polish it a bit and it's ready to go.

* Replace `Result` with `Cancelled` to save an allocation on successful completion.
* Add a test to show that `CancellationException` is never passed to `CoroutineExceptionHandler`.
* Explicitly specify return types.
* Use @JvmField on "package-private" property to not generate an accessor.
Copy link
Contributor Author

@vadimsemenov vadimsemenov left a comment

Choose a reason for hiding this comment

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

Thanks for the review. I added another commit. I'll squash it into the previous one if it looks good to you.

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! Please fix a minor comment and it's good to go.

I'm ready to squash & merge it and make a 1.4.2 release as soon as it's ready

@vadimsemenov
Copy link
Contributor Author

Done.
I also filled #2409 to address the issue with two ignored tests when AbstractFuture.afterDone is finalised.

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