-
Notifications
You must be signed in to change notification settings - Fork 1
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
Specilized API for IO #7
Conversation
@@ -20,7 +20,7 @@ import cats.implicits._ | |||
import munit.CatsEffectSuite | |||
|
|||
class CatchTest extends CatsEffectSuite { | |||
val C = Catch.ioCatch.unsafeRunSync() | |||
val C = Catch.catchForIO.unsafeRunSync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this to avoid confusion between ioCatch and ioCatching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted as we're using IOCatch[E] now
|
||
object IOCatch { | ||
|
||
def ioCatching[E]: IOCatchPartiallyApplied[E] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively can be named catchingIO
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an opinion on apply
?
IOCatch[Error] { h =>
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - that didn't cross my mind 😄 Done
for { | ||
res <- ioCatching[String](h => | ||
IO.uncancelable(poll => poll(h.raise("oops"))) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is interesting because I couldn't get the RaisedInUncancellable exception to be thrown unless I remove the race
in
F.race(prom.get, program).flatMap(_.fold(f, F.pure(_))) |
Perhaps there's a race condition and somehow the outer cancellation always wins? 🤔
Either way, we will need to use raiseError to abort the execution anyway so nothing changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just tried prom.get <* F.sleep(1.second)
and it raised the error.
I think it is very hard to have it throw consistently without being able to query cats-effect about the "current cancelation" context.
Unfortunately I think that also means that the test "Can raise in an uncancellable region" might be flaky 😿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can make both cases pass for now since it's race-y 🤷♂️ Will need to document this behaviour
Happy to add other missing test scenarios if you can think of anything else 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments.
for { | ||
res <- ioCatching[String](h => | ||
IO.uncancelable(poll => poll(h.raise("oops"))) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just tried prom.get <* F.sleep(1.second)
and it raised the error.
I think it is very hard to have it throw consistently without being able to query cats-effect about the "current cancelation" context.
Unfortunately I think that also means that the test "Can raise in an uncancellable region" might be flaky 😿
Some of the |
May I merge @jatcwang ? |
Yes go for it thanks @ValdemarGr! |
Closes #6