Skip to content

Commit ec8b9e0

Browse files
djspiewakdjspiewak
authored
Merge pull request #3453 from djspiewak/bug/canceled-timeout
Corrected handling of self-cancelation within `timeout`
2 parents 4711122 + 0b0f880 commit ec8b9e0

6 files changed

Lines changed: 67 additions & 37 deletions

File tree

‎kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ trait GenSpawn[F[_], E] extends MonadCancel[F, E] with Unique[F] {
352352
case Outcome.Succeeded(fa) => f.cancel *> fa.map(Left(_))
353353
case Outcome.Errored(ea) => f.cancel *> raiseError(ea)
354354
case Outcome.Canceled() =>
355-
poll(f.join).onCancel(f.cancel).flatMap {
355+
f.cancel *> f.join flatMap {
356356
case Outcome.Succeeded(fb) => fb.map(Right(_))
357357
case Outcome.Errored(eb) => raiseError(eb)
358358
case Outcome.Canceled() => poll(canceled) *> never
@@ -363,7 +363,7 @@ trait GenSpawn[F[_], E] extends MonadCancel[F, E] with Unique[F] {
363363
case Outcome.Succeeded(fb) => f.cancel *> fb.map(Right(_))
364364
case Outcome.Errored(eb) => f.cancel *> raiseError(eb)
365365
case Outcome.Canceled() =>
366-
poll(f.join).onCancel(f.cancel).flatMap {
366+
f.cancel *> f.join flatMap {
367367
case Outcome.Succeeded(fa) => fa.map(Left(_))
368368
case Outcome.Errored(ea) => raiseError(ea)
369369
case Outcome.Canceled() => poll(canceled) *> never

‎kernel/shared/src/main/scala/cats/effect/kernel/GenTemporal.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,9 +152,9 @@ trait GenTemporal[F[_], E] extends GenConcurrent[F, E] with Clock[F] {
152152
uncancelable { poll =>
153153
implicit val F: GenTemporal[F, E] = this
154154

155-
racePair(fa, sleep(duration)) flatMap {
155+
poll(racePair(fa, sleep(duration))) flatMap {
156156
case Left((oc, f)) =>
157-
poll(f.cancel *> oc.embedNever)
157+
poll(f.cancel *> oc.embed(poll(F.canceled) *> F.never))
158158

159159
case Right((f, _)) =>
160160
start(f.cancel) *> raiseError[A](ev(new TimeoutException(duration.toString)))

‎kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ sealed abstract class Resource[F[_], +A] extends Serializable {
346346
case Outcome.Errored(ea) =>
347347
F.raiseError[(Either[A, B], ExitCase => F[Unit])](ea).guarantee(cancelLoser(f))
348348
case Outcome.Canceled() =>
349-
poll(f.join).onCancel(f.cancel).flatMap {
349+
f.cancel *> f.join flatMap {
350350
case Outcome.Succeeded(fb) =>
351351
fb.map { case (b, fin) => (Either.right[A, B](b), fin) }
352352
case Outcome.Errored(eb) =>
@@ -369,7 +369,7 @@ sealed abstract class Resource[F[_], +A] extends Serializable {
369369
case Outcome.Errored(eb) =>
370370
F.raiseError[(Either[A, B], ExitCase => F[Unit])](eb).guarantee(cancelLoser(f))
371371
case Outcome.Canceled() =>
372-
poll(f.join).onCancel(f.cancel).flatMap {
372+
f.cancel *> f.join flatMap {
373373
case Outcome.Succeeded(fa) =>
374374
fa.map { case (a, fin) => (Either.left[A, B](a), fin) }
375375
case Outcome.Errored(ea) =>

‎laws/shared/src/main/scala/cats/effect/laws/GenSpawnLaws.scala‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,21 @@ trait GenSpawnLaws[F[_], E] extends MonadCancelLaws[F, E] with UniqueLaws[F] {
3333
case Outcome.Succeeded(fa) => F.productR(f.cancel)(F.map(fa)(Left(_)))
3434
case Outcome.Errored(ea) => F.productR(f.cancel)(F.raiseError(ea))
3535
case Outcome.Canceled() =>
36-
F.flatMap(F.onCancel(poll(f.join), f.cancel)) {
36+
F.flatMap(f.cancel *> f.join) {
3737
case Outcome.Succeeded(fb) => F.map(fb)(Right(_))
3838
case Outcome.Errored(eb) => F.raiseError(eb)
39-
case Outcome.Canceled() => F.productR(F.canceled)(F.never)
39+
case Outcome.Canceled() => F.productR(poll(F.canceled))(F.never)
4040
}
4141
}
4242
case Right((f, oc)) =>
4343
oc match {
4444
case Outcome.Succeeded(fb) => F.productR(f.cancel)(F.map(fb)(Right(_)))
4545
case Outcome.Errored(eb) => F.productR(f.cancel)(F.raiseError(eb))
4646
case Outcome.Canceled() =>
47-
F.flatMap(F.onCancel(poll(f.join), f.cancel)) {
47+
F.flatMap(f.cancel *> f.join) {
4848
case Outcome.Succeeded(fa) => F.map(fa)(Left(_))
4949
case Outcome.Errored(ea) => F.raiseError(ea)
50-
case Outcome.Canceled() => F.productR(F.canceled)(F.never)
50+
case Outcome.Canceled() => F.productR(poll(F.canceled))(F.never)
5151
}
5252
}
5353
}
@@ -64,21 +64,21 @@ trait GenSpawnLaws[F[_], E] extends MonadCancelLaws[F, E] with UniqueLaws[F] {
6464
case Outcome.Succeeded(fa) => F.productR(f.cancel)(F.map(fa)(Left(_)))
6565
case Outcome.Errored(ea) => F.productR(f.cancel)(F.raiseError(ea))
6666
case Outcome.Canceled() =>
67-
F.flatMap(F.onCancel(poll(f.join), f.cancel)) {
67+
F.flatMap(f.cancel *> f.join) {
6868
case Outcome.Succeeded(fb) => F.map(fb)(Right(_))
6969
case Outcome.Errored(eb) => F.raiseError(eb)
70-
case Outcome.Canceled() => F.productR(F.canceled)(F.never)
70+
case Outcome.Canceled() => F.productR(poll(F.canceled))(F.never)
7171
}
7272
}
7373
case Right((f, oc)) =>
7474
oc match {
7575
case Outcome.Succeeded(fb) => F.productR(f.cancel)(F.map(fb)(Right(_)))
7676
case Outcome.Errored(eb) => F.productR(f.cancel)(F.raiseError(eb))
7777
case Outcome.Canceled() =>
78-
F.flatMap(F.onCancel(poll(f.join), f.cancel)) {
78+
F.flatMap(f.cancel *> f.join) {
7979
case Outcome.Succeeded(fa) => F.map(fa)(Left(_))
8080
case Outcome.Errored(ea) => F.raiseError(ea)
81-
case Outcome.Canceled() => F.productR(F.canceled)(F.never)
81+
case Outcome.Canceled() => F.productR(poll(F.canceled))(F.never)
8282
}
8383
}
8484
}
@@ -87,25 +87,23 @@ trait GenSpawnLaws[F[_], E] extends MonadCancelLaws[F, E] with UniqueLaws[F] {
8787
F.race(F.never[A], fb) <-> results
8888
}
8989

90+
@deprecated("law is no longer applicable (or correct)", "3.5.0")
9091
def raceCanceledIdentityLeft[A](fa: F[A]) =
9192
F.race(F.canceled, fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_))) <-> fa.map(
9293
_.asRight[Unit])
9394

95+
@deprecated("law is no longer applicable (or correct)", "3.5.0")
9496
def raceCanceledIdentityRight[A](fa: F[A]) =
9597
F.race(fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_)), F.canceled) <-> fa.map(
9698
_.asLeft[Unit])
9799

98100
def raceNeverNoncanceledIdentityLeft[A](fa: F[A]) =
99-
F.race(F.never[Unit], fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_))) <-> F
100-
.onCancel(
101-
fa.flatMap(r => F.pure(r.asRight[Unit])).handleErrorWith(F.raiseError(_)),
102-
F.never)
101+
F.race(F.never[Unit], fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_))) <->
102+
fa.flatMap(r => F.pure(r.asRight[Unit])).handleErrorWith(F.raiseError(_))
103103

104104
def raceNeverNoncanceledIdentityRight[A](fa: F[A]) =
105-
F.race(fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_)), F.never[Unit]) <-> F
106-
.onCancel(
107-
fa.flatMap(r => F.pure(r.asLeft[Unit])).handleErrorWith(F.raiseError(_)),
108-
F.never)
105+
F.race(fa.flatMap(F.pure(_)).handleErrorWith(F.raiseError(_)), F.never[Unit]) <->
106+
fa.flatMap(r => F.pure(r.asLeft[Unit])).handleErrorWith(F.raiseError(_))
109107

110108
// I really like these laws, since they relate cede to timing, but they're definitely nondeterministic
111109
/*def raceLeftCedeYields[A](a: A) =

‎laws/shared/src/main/scala/cats/effect/laws/GenSpawnTests.scala‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,6 @@ trait GenSpawnTests[F[_], E] extends MonadCancelTests[F, E] with UniqueTests[F]
107107
"race derives from racePair (left)" -> forAll(laws.raceDerivesFromRacePairLeft[A, B] _),
108108
"race derives from racePair (right)" -> forAll(
109109
laws.raceDerivesFromRacePairRight[A, B] _),
110-
"race canceled identity (left)" -> forAll(laws.raceCanceledIdentityLeft[A] _),
111-
"race canceled identity (right)" -> forAll(laws.raceCanceledIdentityRight[A] _),
112110
"race never non-canceled identity (left)" -> forAll(
113111
laws.raceNeverNoncanceledIdentityLeft[A] _),
114112
"race never non-canceled identity (right)" -> forAll(
@@ -197,8 +195,6 @@ trait GenSpawnTests[F[_], E] extends MonadCancelTests[F, E] with UniqueTests[F]
197195
"race derives from racePair (left)" -> forAll(laws.raceDerivesFromRacePairLeft[A, B] _),
198196
"race derives from racePair (right)" -> forAll(
199197
laws.raceDerivesFromRacePairRight[A, B] _),
200-
"race canceled identity (left)" -> forAll(laws.raceCanceledIdentityLeft[A] _),
201-
"race canceled identity (right)" -> forAll(laws.raceCanceledIdentityRight[A] _),
202198
"race never non-canceled identity (left)" -> forAll(
203199
laws.raceNeverNoncanceledIdentityLeft[A] _),
204200
"race never non-canceled identity (right)" -> forAll(

‎tests/shared/src/test/scala/cats/effect/IOSpec.scala‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ package cats.effect
1818

1919
import cats.effect.implicits._
2020
import cats.effect.laws.AsyncTests
21-
import cats.effect.testkit.TestContext
21+
import cats.effect.testkit.{TestContext, TestControl}
2222
import cats.kernel.laws.SerializableLaws.serializable
2323
import cats.kernel.laws.discipline.MonoidTests
2424
import cats.laws.discipline.{AlignTests, SemigroupKTests}
@@ -866,24 +866,24 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification {
866866
Outcome.canceled[IO, Throwable, Unit])
867867
}
868868

869-
"succeed if lhs cancels" in ticked { implicit ticker =>
870-
IO.race(IO.canceled, IO.pure(1)) must completeAs(Right(1))
869+
"cancel if lhs cancels and rhs succeeds" in ticked { implicit ticker =>
870+
IO.race(IO.canceled, IO.sleep(1.milli) *> IO.pure(1)).void must selfCancel
871871
}
872872

873-
"succeed if rhs cancels" in ticked { implicit ticker =>
874-
IO.race(IO.pure(1), IO.canceled) must completeAs(Left(1))
873+
"cancel if rhs cancels and lhs succeeds" in ticked { implicit ticker =>
874+
IO.race(IO.sleep(1.milli) *> IO.pure(1), IO.canceled).void must selfCancel
875875
}
876876

877-
"fail if lhs cancels and rhs fails" in ticked { implicit ticker =>
877+
"cancel if lhs cancels and rhs fails" in ticked { implicit ticker =>
878878
case object TestException extends Throwable
879-
IO.race(IO.canceled, IO.raiseError[Unit](TestException)).void must failAs(
880-
TestException)
879+
IO.race(IO.canceled, IO.sleep(1.milli) *> IO.raiseError[Unit](TestException))
880+
.void must selfCancel
881881
}
882882

883-
"fail if rhs cancels and lhs fails" in ticked { implicit ticker =>
883+
"cancel if rhs cancels and lhs fails" in ticked { implicit ticker =>
884884
case object TestException extends Throwable
885-
IO.race(IO.raiseError[Unit](TestException), IO.canceled).void must failAs(
886-
TestException)
885+
IO.race(IO.sleep(1.milli) *> IO.raiseError[Unit](TestException), IO.canceled)
886+
.void must selfCancel
887887
}
888888

889889
"cancel both fibers" in ticked { implicit ticker =>
@@ -912,6 +912,42 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification {
912912
}
913913
}
914914

915+
"immediately cancel when timing out canceled" in real {
916+
val program = IO.canceled.timeout(2.seconds)
917+
918+
val test = TestControl.execute(program.start.flatMap(_.join)) flatMap { ctl =>
919+
ctl.tickFor(1.second) *> ctl.results
920+
}
921+
922+
test flatMap { results =>
923+
IO {
924+
results must beLike { case Some(Outcome.Succeeded(Outcome.Canceled())) => ok }
925+
}
926+
}
927+
}
928+
929+
"immediately cancel when timing out and forgetting canceled" in real {
930+
val program = IO.canceled.timeoutAndForget(2.seconds)
931+
val test = TestControl.execute(program.start.flatMap(_.join)) flatMap { ctl =>
932+
ctl.tickFor(1.second) *> ctl.results
933+
}
934+
935+
test flatMap { results =>
936+
IO {
937+
results must beLike { case Some(Outcome.Succeeded(Outcome.Canceled())) => ok }
938+
}
939+
}
940+
}
941+
942+
"timeout a suspended timeoutAndForget" in real {
943+
val program = IO.never.timeoutAndForget(2.seconds).timeout(1.second)
944+
val test = TestControl.execute(program.start.flatMap(_.join)) flatMap { ctl =>
945+
ctl.tickFor(1.second) *> ctl.results
946+
}
947+
948+
test.flatMap(results => IO(results must beSome))
949+
}
950+
915951
"return the left when racing against never" in ticked { implicit ticker =>
916952
IO.pure(42)
917953
.racePair(IO.never: IO[Unit])

0 commit comments

Comments
 (0)