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

Specilized API for IO #7

Merged
merged 7 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/core/src/main/scala/catcheffect/Catch.scala
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ object Catch {
|The handler was defined at $alloc""".stripMargin
}

def ioCatch: IO[Catch[IO]] =
val catchForIO: IO[Catch[IO]] =
LocalForIOLocal
.localForIOLocalDefault(Vault.empty)
.map(implicit loc => fromLocal[IO])
Expand Down
20 changes: 20 additions & 0 deletions modules/core/src/main/scala/catcheffect/IOCatch.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package catcheffect

import Catch.catchForIO
import cats.effect.*

object IOCatch {

def ioCatching[E]: IOCatchPartiallyApplied[E] =
Copy link
Contributor Author

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?

Copy link
Owner

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 =>
  // ...
}

Copy link
Contributor Author

@jatcwang jatcwang Jul 2, 2024

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

new IOCatchPartiallyApplied[E]

class IOCatchPartiallyApplied[E] {
def apply[A](f: Handle[IO, E] => IO[A]): IO[Either[E, A]] = {
catchForIO.flatMap { c =>
c.use[E](f)
}
}
}

}

2 changes: 1 addition & 1 deletion modules/core/src/test/scala/catcheffect/CatchTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import cats.implicits._
import munit.CatsEffectSuite

class CatchTest extends CatsEffectSuite {
val C = Catch.ioCatch.unsafeRunSync()
val C = Catch.catchForIO.unsafeRunSync()
Copy link
Contributor Author

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

Copy link
Contributor Author

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


test("should be able to throw, catch and re-throw") {
val program = C.use[String] { hs =>
Expand Down
68 changes: 68 additions & 0 deletions modules/core/src/test/scala/catcheffect/IOCatchTest.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package catcheffect

import catcheffect.Catch.RaisedWithoutHandler
import catcheffect.IOCatch.ioCatching
import cats.effect.*
import munit.CatsEffectSuite

class IOCatchTest extends CatsEffectSuite {
test("should abort execution as soon as raise is called") {
for {
ref <- Ref[IO].of("init")
res <- ioCatching[String](h =>
for {
_ <- h.raise("error")
_ <- ref.set("should not be set")
} yield 1
)
_ = assertEquals(res, Left("error"))
_ <- assertIO(ref.get, "init")
} yield ()
}

test("can nest correctly") {
for {
res <- ioCatching[String](hs1 =>
for {
_ <- ioCatching[String](hs2 =>
hs1.raise("1")
)
_ <- IO.raiseError(new AssertionError("should not reach this point"))
} yield ()
)
_ = assertEquals(res, Left("1"))
} yield res
}

test("Can raise in an uncancellable region") {
for {
res <- ioCatching[String](h =>
IO.uncancelable(_ => h.raise("oops"))
)
_ = assertEquals(res, Left("oops"))
} yield ()
}

test("Can raise in an cancellable region when polled") {
for {
res <- ioCatching[String](h =>
IO.uncancelable(poll => poll(h.raise("oops")))
)
Copy link
Contributor Author

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.

Copy link
Owner

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 😿

Copy link
Contributor Author

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

_ = assertEquals(res, Left("oops"))
} yield ()
}

test("Fail to raise if the Raise instance is used outside of its original fiber") {
(for {
cached <- Deferred[IO, Raise[IO, String]]
res <- ioCatching[String](h =>
cached.complete(h).void
)
_ = assertEquals(res, Right(()))
_ <- cached.get.flatMap(_.raise("hm"))
} yield ()).attempt.map {
case Left(_: RaisedWithoutHandler[_]) => () // succeed
case other => fail(s"Expected RaisedWithoutHandler, got $other")
}
}
}
Loading