Repaired some of ListenableFuture.kt's cancellation corner cases. #1441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes:
- Cancellation without an untrapped CancellationException propagating
through a Callback; isCancelled() is the correct way to check for
cancellation
- Bidirectional propagation of cancellation through
`asListenableFuture()`
- The cause getting lost in the `asListenableFuture()` future when
cancelling its `Deferred` parent with a cause
This also:
- Extensively documents the package and the contracts created by the
promise-creating extension methods and `future()`
- Uses `getUninterruptibly()` for speed
- Uses `AbstractFuture` to make as certain as possible that
`Future.cancel()` will return `true` at most once
- Should clear up rare spooky race conditions around
cancellation/interruption in hybrid Coroutines/Guava Futures
codebases
There are probably a few more interesting corner cases hiding in here,
but this should be a good start improving the correctness of `.guava`'s
adapters.
This is a squash commit of kotlin/pr/1347, rebased on develop:
- Incorporated first-round feedback.
- Merged CancellationToCoroutine into ListenableFutureCoroutine to save an
allocation.
- Documented and tested for null completion of asDeferred()'s parent
Future.
- Renamed a cancellation test case for clarity of purpose.
- Split asDeferred() documentation between KDoc/details
- Implemented InternalFutures faster-fast path. Documented.
|
Filed a followup issue so |
lowasser
reviewed
Sep 3, 2019
| @@ -38,81 +47,98 @@ public fun <T> CoroutineScope.future( | |||
| ): ListenableFuture<T> { | |||
| require(!start.isLazy) { "$start start is not supported" } | |||
| val newContext = newCoroutineContext(context) | |||
| // TODO: It'd be nice not to leak this SettableFuture reference, which is easily blind-cast. | |||
There was a problem hiding this comment.
A nice short way to do this, if you like: return object: ListenableFuture<T> by future {}.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes:
through a Callback; isCancelled() is the correct way to check for
cancellation
asListenableFuture()asListenableFuture()future whencancelling its
Deferredparent with a causeThis also:
promise-creating extension methods and
future()getUninterruptibly()for speedAbstractFutureto make as certain as possible thatFuture.cancel()will returntrueat most oncecancellation/interruption in hybrid Coroutines/Guava Futures
codebases
There are probably a few more interesting corner cases hiding in here,
but this should be a good start improving the correctness of
.guava'sadapters.
This only addresses the
asFoo()adapters, not thefuture()direct path. That also needs some cancellation changes. TBD.This is a squash commit of kotlin/pr/1347 with some more improvements, rebased on develop:
Incorporated first-round feedback.
Merged CancellationToCoroutine into ListenableFutureCoroutine to save an
allocation.
Documented and tested for null completion of asDeferred()'s parent
Future.
Renamed a cancellation test case for clarity of purpose.
Split asDeferred() documentation between KDoc/details
Implemented InternalFutures faster-fast path. Documented.