From 333ec283126138019f455fd8f85eb38200b3b29e Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Thu, 15 Aug 2024 23:34:08 +0000 Subject: [PATCH 1/4] Add tests for `cancelable` bug with non-happy-path finalizer --- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index e8ec582c43..6c300fc266 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1145,6 +1145,17 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { } must completeAs(()) } + "cancelable waits for termination if finalizer errors" in ticked { implicit ticker => + val test = IO.never.uncancelable.cancelable(IO.raiseError(new Exception)) + test.start.flatMap(IO.sleep(1.second) *> _.cancel) must nonTerminate + } + + "cancelable waits for termination if finalizer self-cancels" in ticked { + implicit ticker => + val test = IO.never.uncancelable.cancelable(IO.canceled) + test.start.flatMap(IO.sleep(1.second) *> _.cancel) must nonTerminate + } + "only unmask within current fiber" in ticked { implicit ticker => var passed = false val test = IO uncancelable { poll => From 10079e34ec6a1fc9f07fdbed952532f2e14441e3 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Fri, 16 Aug 2024 23:38:33 +0000 Subject: [PATCH 2/4] Refactor `cancelable` tests --- .../src/test/scala/cats/effect/IOSpec.scala | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 6c300fc266..e4ccc13a2e 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1145,15 +1145,27 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { } must completeAs(()) } - "cancelable waits for termination if finalizer errors" in ticked { implicit ticker => - val test = IO.never.uncancelable.cancelable(IO.raiseError(new Exception)) - test.start.flatMap(IO.sleep(1.second) *> _.cancel) must nonTerminate + "cancelable waits for termination" in ticked { implicit ticker => + def test(fin: IO[Unit]) = { + val go = IO.never.uncancelable.cancelable(fin) + go.start.flatMap(IO.sleep(1.second) *> _.cancel) + } + + test(IO.unit) must nonTerminate + test(IO.raiseError(new Exception)) must nonTerminate + test(IO.canceled) must nonTerminate } - "cancelable waits for termination if finalizer self-cancels" in ticked { - implicit ticker => - val test = IO.never.uncancelable.cancelable(IO.canceled) - test.start.flatMap(IO.sleep(1.second) *> _.cancel) must nonTerminate + "cancelable cancels task" in ticked { implicit ticker => + def test(fin: IO[Unit]) = + IO.deferred[Unit].flatMap { latch => + val go = IO.never[Unit].onCancel(latch.complete(()).void).cancelable(fin) + go.start.flatMap(IO.sleep(1.second) *> _.cancel) *> latch.get + } + + test(IO.unit) must completeAs(()) + test(IO.raiseError(new Exception)) must completeAs(()) + test(IO.canceled) must completeAs(()) } "only unmask within current fiber" in ticked { implicit ticker => From 981a127342c70f745271b5df3b94527bc92db837 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Fri, 16 Aug 2024 23:41:05 +0000 Subject: [PATCH 3/4] Fix `cancelable` to `guarantee` cancelation --- kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala b/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala index d4d8d9ab6e..306579750f 100644 --- a/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala +++ b/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala @@ -264,7 +264,7 @@ trait GenSpawn[F[_], E] extends MonadCancel[F, E] with Unique[F] { def cancelable[A](fa: F[A], fin: F[Unit]): F[A] = uncancelable { poll => start(fa) flatMap { fiber => - poll(fiber.join).onCancel(fin *> fiber.cancel).flatMap(_.embed(poll(canceled *> never))) + poll(fiber.join).onCancel(fin.guarantee(fiber.cancel)).flatMap(_.embed(poll(canceled *> never))) } } From ca499c5fea24cc5f32b9331d472bba21d531336c Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Fri, 16 Aug 2024 23:46:44 +0000 Subject: [PATCH 4/4] Formatting --- .../shared/src/main/scala/cats/effect/kernel/GenSpawn.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala b/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala index 306579750f..7d5737afb1 100644 --- a/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala +++ b/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala @@ -264,7 +264,9 @@ trait GenSpawn[F[_], E] extends MonadCancel[F, E] with Unique[F] { def cancelable[A](fa: F[A], fin: F[Unit]): F[A] = uncancelable { poll => start(fa) flatMap { fiber => - poll(fiber.join).onCancel(fin.guarantee(fiber.cancel)).flatMap(_.embed(poll(canceled *> never))) + poll(fiber.join) + .onCancel(fin.guarantee(fiber.cancel)) + .flatMap(_.embed(poll(canceled *> never))) } }