Fix fatal error propagation in async operations #4512
Conversation
|
Hi @armanbilge, @durban Would you mind taking a look at this when you have a moment? Thanks! |
armanbilge
left a comment
armanbilge
left a comment
There was a problem hiding this comment.
Thanks for the PR!
- Since this is a bug fix, you can target the
series/3.6.xbranch - Can you add a test for a minimized version of the issue? You may need to add it to
IOAppSuitebecause this deals with fatal errors.
| * @param fut | ||
| * The `java.util.concurrent.CompletableFuture` to suspend in `F[_]` | ||
| */ | ||
| def fromCompletableFuture[A](fut: F[CompletableFuture[A]]): F[A] = cont { |
There was a problem hiding this comment.
Are changes to this method necessary? #4505 (comment) suggests that the problem is actually in async and not directly in fromCompletableFuture.
There was a problem hiding this comment.
That's right that async_ is the core issue, but i think both changes are needed:
fromCompletableFuture: Uses CompletableFuture.handle() which catches all exceptions, including fatal ones which without fix, OutOfMemoryError gets wrapped instead of crashing the JVM.
async_: The IO.delay() wrapper was catching fatal errors during callback registration, causing the same problem.
Both mechanisms need the fix to ensure fatal errors consistently crash the JVM.
There was a problem hiding this comment.
I added more insights on how the error re-surface from onError here: #4505 (comment)
There was a problem hiding this comment.
Thanks @tpetillot
I added more insights on how the error re-surface from
onErrorhere: #4505 (comment)
looks like this code (Attached on the comment) is not detecting the original fatal error, but it's actually:
- Detecting errors thrown BY the error handler
(f(error))but NOT detecting the original fatal error that triggeredonError
The fatal error detection happens after the onError machinery, not during it.
I need to be guided if that is right 😊
There was a problem hiding this comment.
Yep, definitely missed that, tried to draw the route of the error outcome for OOM with IO.cont here: #4505 (comment)
There was a problem hiding this comment.
What do you think about the handling fatal failure (onFatalFailure) in failed to ensure the error is treated properly on consumption?
najuna-brian
force-pushed
the
fix/fatal-error-propagation
branch
from
4b68700 to
ee0f110
Compare
Thanks for the review and guidance. Just learnt about all these upstream branches now 😊 |
|
Hello @armanbilge |
|
@najuna-brian #4518 is merged now, thanks for your patience! You can merge the latest |
armanbilge
left a comment
armanbilge
left a comment
There was a problem hiding this comment.
@najuna-brian it looks like one of the tests you added is hanging in the CI. Are you able to run the tests locally?
| h.stderr() must contain("Boom from async!") | ||
| h.stdout() must not(contain("sadness")) | ||
| } else { | ||
| // Fatal error testing is JVM-only |
There was a problem hiding this comment.
Hmm, are you sure? I am pretty sure we support fatal errors on all platforms.
Yes true they keep hanging, Though I am not sure of how to solve that. |
Thank you @armanbilge |
You should start by identifying which test is hanging (a low-tech way to do this is to comment out tests until you figure out which one it is). Then, once you know which test it is, you can try reverting some of your changes to identify which change you made may cause it to hang. |
3 participants
Description
Fixes #4505
Fixed fatal error propagation in async operations so fatal errors crash the JVM instead of being caught and wrapped.
Changes
kernel/jvm/src/main/scala/cats/effect/kernel/AsyncPlatform.scala: Added fatal error detection infromCompletableFutureto re-throw fatal errors instead of wrapping themcore/shared/src/main/scala/cats/effect/IO.scala: RemovedIO.delay()wrapper inIO.async_to allow fatal errors to propagate during callback registrationWhy both changes?
fromCompletableFutureusesCompletableFuture.handle()which catches all exceptions, including fatal onesasync_hadIO.delay()wrapper catching fatal errors during callback registrationTesting
fromCompletableFutureandasync_fatal error scenarios.onError(_ => IO.unit)workaround mentioned in the issue