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

Breaking change: Guava future coroutine builder shouldn't report to CoroutineExceptionHandler #2840

Merged
merged 2 commits into from Sep 30, 2021

Conversation

vadimsemenov
Copy link
Contributor

Fixes #2774 and #2791.

This change makes future coroutine builder consistent with java.util.concurrent.FutureTask which also drops exceptions that happen after successful cancellation.

Note: future will still pass the error to its parent in the tree if there's one. But if there's no parent, the exception is dropped.

…oroutineExceptionHandler.

See  Kotlin#2774 and Kotlin#2791 for more context.

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happens after successful cancellation.
@vadimsemenov
Copy link
Contributor Author

Gentle ping :)

@qwwdfsad qwwdfsad self-requested a review September 27, 2021 12:10
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.

Thanks for the detailed documentation and fix! The changes look good to me.

Is it okay to ask you to do the same in CompletableFuture builder within this PR?
I suspect it is prone to the same issue.
It's completely okay if you are short on time, your contribution is valuable and appreciated as is.

@vadimsemenov
Copy link
Contributor Author

I can look into CompletableFuture next week.
For some reason testFutureDoesNotReportToCoroutineExceptionHandler doesn't fail when ported to CompletableFuture.
If you don't mind, I'd send a separate PR for CompletableFuture.

@qwwdfsad
Copy link
Member

Sure, thanks for taking care of it!

@qwwdfsad qwwdfsad merged commit 60eefec into Kotlin:develop Sep 30, 2021
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this pull request Oct 14, 2021
…oroutineExceptionHandler (Kotlin#2840)

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happen after successful cancellation.

Fixes Kotlin#2774
Fixes Kotlin#2791
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
…oroutineExceptionHandler (Kotlin#2840)

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happen after successful cancellation.

Fixes Kotlin#2774
Fixes Kotlin#2791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants