Immediately surface fatal errors in IO.raiseError
#3811
Merged
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
armanbilge
reviewed
Sep 6, 2023
Comment on lines
350
to
361
| import scala.util.control.NonFatal | ||
| val io = IO.raiseError[Unit](new VirtualMachineError {}).voidError | ||
|
|
||
| val fatalThrown = | ||
| try { | ||
| unsafeRun[Unit](io) | ||
| false | ||
| } catch { | ||
| case t if NonFatal(t) => false | ||
| case _: Throwable => true | ||
| } | ||
| IO(fatalThrown) must completeAs(true) |
There was a problem hiding this comment.
Suggested change
| import scala.util.control.NonFatal | |
| val io = IO.raiseError[Unit](new VirtualMachineError {}).voidError | |
| val fatalThrown = | |
| try { | |
| unsafeRun[Unit](io) | |
| false | |
| } catch { | |
| case t if NonFatal(t) => false | |
| case _: Throwable => true | |
| } | |
| IO(fatalThrown) must completeAs(true) | |
| val error = new VirtualMachineError {} | |
| val io = IO.raiseError[Unit](error).voidError | |
| val fatalThrown = | |
| try { | |
| unsafeRun[Unit](io) | |
| false | |
| } catch { | |
| case t: Throwable => t eq error | |
| } | |
| IO.pure(fatalThrown) must completeAs(true) |
armanbilge
reviewed
Sep 7, 2023
Comment on lines
244
to
247
| if (!NonFatal(cur.t)) | ||
| onFatalFailure(cur.t) | ||
|
|
||
| runLoop(failed(cur.t, 0), nextCancelation, nextAutoCede) |
There was a problem hiding this comment.
Suggested change
| if (!NonFatal(cur.t)) | |
| onFatalFailure(cur.t) | |
| runLoop(failed(cur.t, 0), nextCancelation, nextAutoCede) | |
| val ex = cur.t | |
| if (!NonFatal(ex)) | |
| onFatalFailure(ex) | |
| runLoop(failed(ex, 0), nextCancelation, nextAutoCede) |
|
CI seems to be hanging, and it looks legit. Not sure what's going on exactly ... |
|
I played with this a bit last night. The fix and test seem good but somehow the test runner just hangs. I don't really understand what's happening, I assume the fatal exception is escaping somehow and killing a thread. Maybe it's a specs2 thing, not sure ... |
|
Wait, doesn't |
|
Oh LOL good point. We should make a new runtime for this? On the other hand, I'm pretty sure |
|
Haha yup, that would be it. cats-effect/core/shared/src/main/scala/cats/effect/IOFiber.scala Lines 1547 to 1552 in 1bbe7f6 Ok, so we need to rewrite this as an Those live here: https://github.com/typelevel/cats-effect/blob/1bbe7f619125302131b0d6c94a67ad05aa31e33a/tests/shared/src/main/scala/catseffect/examples.scala Then we can add the test to |
|
just out of curiosity, how did you debug it to discover where the error was? |
|
@diogocanut who are you addressing? I'm not sure if anyone "debugged" it per se. Daniel Urban reminded me that fatal errors shutdown the runtime, and so I just checked the implementation of |
|
I mean @scott-thomson239 . I tried looking at this but didn't went through the point of going to |
|
@diogocanut I happened to watch this talk by Daniel Spiewak which gives a very nice overview of |
… attempt case in runloop
|
It seems there were a couple of other places in There are two other cases in the FlatMap branch and Map branch but I didn't add fatal error handling here since I can't see any scenario where a fatal error could be encountered here without it having been handled earlier. |
|
@scott-thomson239 I've started CI, and it seems it failed with scalafmt. (Next time feel free to comment if CI is not approved in a timely manner.) |
|
Looks like some legitimate failures on Node.js. |
|
The new example cats-effect/tests/js/src/main/scala/catseffect/examplesplatform.scala Lines 44 to 59 in c75d6a2 |
|
I've registered the new |
Ah, I'm sorry, we haven't enabled those tests yet on this branch. We'll have to fix that when we merge into |
armanbilge
reviewed
Sep 23, 2023
There was a problem hiding this comment.
I think there are two more places we need to handle this.
cats-effect/core/shared/src/main/scala/cats/effect/IOFiber.scala
Lines 320 to 322 in 6b59c54
cats-effect/core/shared/src/main/scala/cats/effect/IOFiber.scala
Lines 387 to 389 in 6b59c54
armanbilge
approved these changes
Sep 25, 2023
Comment on lines
448
to
+450
| val t = error.t | ||
| if (!NonFatal(t)) | ||
| onFatalFailure(t) |
There was a problem hiding this comment.
Woops, looks like the convention was val t = error.t and not val ex = error.t. Not a big deal :)
djspiewak
approved these changes
Sep 26, 2023
BeforeAfter |
|
Getting back around to analyzing the benchmark… Almost zero change outside the margin of error. Where it is changed, this branch is faster (weird?!), so I call that a win! |
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.
Resolves #3767
From my understanding, the problem was that manually raising fatal errors through 'raiseError' was not treated the same as a fatal error thrown inside a delay within
IOFiber. This PR just adds a check in theErrorcase to callonFatalFailureif a raised error is a fatal error.Sorry this has taken so long. I was struggling to figure out how to test a fatal error is thrown without it crashing the test until I discovered
unsafeRunexists.