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

Structured concurrency for Completable/Listenable futures #1007

Closed
elizarov opened this issue Feb 25, 2019 · 0 comments
Closed

Structured concurrency for Completable/Listenable futures #1007

elizarov opened this issue Feb 25, 2019 · 0 comments

Comments

@elizarov
Copy link
Contributor

elizarov commented Feb 25, 2019

kotlinx-coroutines-jdk8and kotlinx-coroutines-guava integration modules' future { ... } builders do not honor structured concurrency in the same way as all other builders -- a failure inside the child (builder) code did not cancel the parent coroutine. This change is to address this discrepancy and to bring them inline with all the the other builders, both core ones (launch, async, produce, etc) and from other integration modules (rxSingle, publish, etc).

Note that this change does not affect non-structured (typical) usage like GlobalScope.future { ... }.

Please note, that there is an open design issue (#763) regarding async { ... } and this change "deepends" this problem by bringing future { ... } inline with this structured concurrency behaviour. However, this is not a major issue for future { ... } since it is integration vehicle that is typically used to adapt suspending function to the Java code that expects a future result like this:

suspend fun doSomething(): T { .... } // Kotlin fun

fun doSomethingAsync(): CompletableFuture<T> = GlobalScope.future { // Java fun
     doSomething()
}
elizarov added a commit that referenced this issue Feb 25, 2019
BREAKING BEHAVIOR CHANGE:

* kotlinx-coroutines-jdk8 and -guava integration modules
  future { ... } builders now honor structured concurrency in
  the same way as all other builders -- a failure inside the child
  (builder) code now cancels parent coroutine.
  Note that is does not affect non-structured (typical) usage like
  GlobalScope.future { ... }

MINOR BEHAVIOR CHANGE:

* Exception in installed CancellableCoroutine.invokeOnCancellation
  handler does not cancel the parent job, but is considered to be an
  uncaught exception, so it goes to CoroutineExceptionHandler.

Internal changes:

* JobSupport.cancelsParents=true is now a default, since there are
  only a fewer exceptions for builder that throw their exception from block
* JobSupport.handleJobException has additional "handled" parameter to
  distinguish cases when parent did/did-not handle it.
* handleCoroutineException logic is updated. It never cancels parent,
  since parent cancellation is taken care of by structured concurrency.
* handleCoroutineException is always invoked with current coroutine's
  context (as opposed to parent)

Fixes #1007
elizarov added a commit that referenced this issue Feb 25, 2019
BREAKING BEHAVIOR CHANGE:

* kotlinx-coroutines-jdk8 and -guava integration modules
  future { ... } builders now honor structured concurrency in
  the same way as all other builders -- a failure inside the child
  (builder) code now cancels parent coroutine.
  Note that is does not affect non-structured (typical) usage like
  GlobalScope.future { ... }

MINOR BEHAVIOR CHANGE:

* Exception in installed CancellableCoroutine.invokeOnCancellation
  handler does not cancel the parent job, but is considered to be an
  uncaught exception, so it goes to CoroutineExceptionHandler.

Internal changes:

* JobSupport.cancelsParents=true is now a default, since there are
  only a fewer exceptions for builder that throw their exception from block
* JobSupport.handleJobException has additional "handled" parameter to
  distinguish cases when parent did/did-not handle it.
* handleCoroutineException logic is updated. It never cancels parent,
  since parent cancellation is taken care of by structured concurrency.
* handleCoroutineException is always invoked with current coroutine's
  context (as opposed to parent)

Fixes #1007
@elizarov elizarov self-assigned this Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant