Handle self-cancellation when running fibers to avoid deadlocks #1484

wemrysi · 2020-12-06T21:19:52Z

As discussed in #1455, we'd like to handle self-cancellation when running fibers to avoid deadlocks. This introduces IO#unsafeRunAsyncOutcome to explicitly expose the case where the running fiber the caller started was canceled. The existing unsafeRun* variants now raise a CancellationException if they are canceled, ensuring the callback is always invoked on termination.

A result of this change is that cancellation and non-termination are now distinguished in tests. This distinction resulted in a failure of GenSpawnLaws#uncancelableRaceDisplacesCanceled which, upon inspection, seems like it should be expecting F.never instead of F.canceled.

Looking at the other uncancelableRace* laws, they seem to imply the mask status of a fiber is inherited by fibers it spawns, is that the intended semantic? It doesn't appear to be the current semantic of IO, as illustrated by

var flag = 0
val flagger = IO.canceled.onCancel(IO { flag = -1 }).flatMap(_ => IO { flag = 1 })

// A
flag = 0
flagger.unsafeRunSync() // flag = -1

// B
flag = 0
IO.uncancelable(_ => flagger).unsafeRunSync() // flag = 1

// C
flag = 0
// unsafeRunTimed as both sides of the race are canceled which is defined to be non-termination when the race is masked
IO.uncancelable(_ => IO.race(flagger, IO.canceled)).unsafeRunTimed(1.second) // flag = -1

// D
flag = 0
// from uncancelableRacePollCanceledIdentityRight
IO.uncancelable(p => IO.race(flagger, p(IO.canceled))).unsafeRunTimed(1.second) // flag = -1

Generally, any time fa is canceled, the lhs of those laws will not terminate (by definition of GenSpawn#race) whereas the rhs will terminate as canceled (and prior to this PR, the tests considered those outcomes equal). Even if we take that into account via unsafeRunTimed, the laws stay that D <-> B when actually D <-> A.

Thoughts? Have I overlooked something or made poor assumptions?

…tests

djspiewak · 2020-12-07T04:41:58Z

and prior to this PR, the tests considered those outcomes equal

Wow, I didn't realize this. This is a rather significant oversight in the test harnessing.

Working back to your earlier points…

Looking at the other uncancelableRace* laws, they seem to imply the mask status of a fiber is inherited by fibers it spawns, is that the intended semantic?

That's definitely not the intended semantic anymore. There was a time where it was what we were shooting for. race is now intended to simply be a mechanism for starting and simultaneously joining a pair of fibers, which is why it is immediately non-primitive within IO. It's possible though that this was an oversight caused by the fact that the tests just… never started failing.

djspiewak

This looks really really nice.

seems like it should be expecting F.never instead of F.canceled.

You're correct! Documentation and implementation agree:

            case Outcome.Canceled() =>
              poll(f.join).onCancel(f.cancel).flatMap {

I believe that the laws should be corrected.

wemrysi · 2020-12-13T22:34:44Z

laws/shared/src/main/scala/cats/effect/laws/AsyncLaws.scala

@@ -26,11 +26,13 @@ import scala.util.{Left, Right}
 trait AsyncLaws[F[_]] extends GenTemporalLaws[F, Throwable] with SyncLaws[F] {
  implicit val F: Async[F]

-  def asyncRightIsSequencedPure[A](a: A, fu: F[Unit]) =
-    F.async[A](k => F.delay(k(Right(a))) >> fu.as(None)) <-> (fu >> F.pure(a))
+  def asyncRightIsUncancelableSequencedPure[A](a: A, fu: F[Unit]) =


My rationale for the uncancelable is that fu is sequenced when async is evaluated, but its cancellation status doesn't affect the outcome.

Actually I think it's cancelation status still affects the outcome. fu = F.canceled would still imply F.canceled on both sides. Does this actually produce a different test outcome?

You're right, good call, looks like I misinterpreted the failures before. Running the original laws again shows they fail when one side doesn't terminate and the other ends up as canceled. I'll have to investigate that more.

Looking into this a bit more, I think this change is actually correct if we want to keep the current semantic that the effect returned from the async callback

(Either[Throwable, A] => Unit) => F[Option[F[Unit]]]

is uncancelable (e.g. consider fu = F.canceled >> F.never).

I think my use of the term "outcome" in the OP caused confusion. fu does affect the resulting Outcome, but not the "outcome" in the sense that cancellation is suppressed.

wemrysi · 2020-12-13T22:36:09Z

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

-  def uncancelableRaceDisplacesCanceled =
-    F.uncancelable(_ => F.race(F.never[Unit], F.canceled)).void <-> F.canceled
-
-  def uncancelableRacePollCanceledIdentityLeft[A](fa: F[A]) =


These no longer seemed pertinent given the current desired semantics of race.

wemrysi · 2020-12-13T23:10:06Z

@djspiewak Ready for another look. I've corrected or removed (justification in comments above) some of the laws that began failing due to distinguishing between non-termination and failure or to the presence of previously ungenerated constructs.

There are other failing laws/tests that I've commented out with a FIXME so that the build is clean, allowing this PR to be merged so others can benefit from the changes to the testing environment, if desired.

The remaining offenders are

IOSpec: "round trip through s.c.Future"
jvm/IOPlatformSpecification: "round trip through j.u.c.CompletableFuture"
GenSpawnTests
- "async.race canceled identity (left)"
- "async.race canceled identity (right)"
MemoizeSpec: "Concurrent.memoize and then flatten is identity"

the Future ones fail as we don't distinguish cancellation when converting to futures, though the tests seem like they'd still be worthwhile limited to uncancelable values (maybe we add some additional test that assert cancellation converts as an error). The others I haven't had a chance to investigate thoroughly yet.

djspiewak · 2020-12-18T15:38:38Z

Sorry for the delay on getting to this! It's been a hell of a week… Churning through it now. Also thank you for taking this on!

djspiewak · 2020-12-18T18:35:06Z

laws/shared/src/main/scala/cats/effect/laws/AsyncLaws.scala

@@ -26,11 +26,13 @@ import scala.util.{Left, Right}
 trait AsyncLaws[F[_]] extends GenTemporalLaws[F, Throwable] with SyncLaws[F] {
  implicit val F: Async[F]

-  def asyncRightIsSequencedPure[A](a: A, fu: F[Unit]) =
-    F.async[A](k => F.delay(k(Right(a))) >> fu.as(None)) <-> (fu >> F.pure(a))
+  def asyncRightIsUncancelableSequencedPure[A](a: A, fu: F[Unit]) =


Actually I think it's cancelation status still affects the outcome. fu = F.canceled would still imply F.canceled on both sides. Does this actually produce a different test outcome?

djspiewak · 2020-12-18T18:36:36Z

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

-  def raceNeverIdentityLeft[A](fa: F[A]) =
-    F.race(F.never[Unit], fa) <-> fa.map(_.asRight[Unit])
-
-  def raceNeverIdentityRight[A](fa: F[A]) =
-    F.race(fa, F.never[Unit]) <-> fa.map(_.asLeft[Unit])


These actually seem like pretty important laws. I take it that they fall apart when fa = F.canceled?

Yeah, exactly.

We can fix this by using .onCancel(F.never) again. That's not quite an identity, and it loses some generality, but it does allow us to talk about task completion and race once again.

djspiewak · 2020-12-18T18:36:51Z

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

-  def uncancelableRaceDisplacesCanceled =
-    F.uncancelable(_ => F.race(F.never[Unit], F.canceled)).void <-> F.canceled
-
-  def uncancelableRacePollCanceledIdentityLeft[A](fa: F[A]) =


djspiewak · 2020-12-18T18:39:25Z

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

-    F.uncancelable(p => F.race(fa, p(F.canceled))) <-> F.uncancelable(_ =>
-      fa.map(_.asLeft[Unit]))
+  def uncancelableRaceNotInherited =
+    F.uncancelable(_ => F.race(F.never[Unit], F.canceled)).void <-> F.never[Unit]


I'd rather be a bit more general here:

F.uncancelable(_ => F.race(fa, fb)) <-> F.race(fa, fb).onCancel(F.never)

djspiewak · 2020-12-18T18:39:46Z

project/plugins.sbt

@@ -1,6 +1,6 @@
 libraryDependencies += "org.scala-js" %% "scalajs-env-selenium" % "1.1.0"

-addSbtPlugin("com.codecommit"     % "sbt-spiewak-sonatype"     % "0.18.3")
+addSbtPlugin("com.codecommit"     % "sbt-spiewak-sonatype"     % "0.19.2")


🎉 Not having scala steward on branches is pretty painful.

djspiewak · 2020-12-18T22:57:55Z

@wemrysi I think we're going to merge this aggressively (since it reveals a bunch of bugs which are currently invisible) and follow-up with further law refinements and fixes. Gucci?

wemrysi · 2020-12-18T23:27:42Z

@djspiewak I was planning to fix the two that you called out in review later today, want to wait until those are done?

djspiewak · 2020-12-18T23:29:28Z

@wemrysi I'm mostly thinking of the fact that we want to get another milestone out with support for 3.0.0-M3, and I'd rather fold your changes into it. Since the follow-ups are bug fixes (#1519) and law adjustments, it feels like we can release the milestone without them, but if you feel it's worth a bit of delay I'm cool with it unless @mpilquist rains fire on us.

wemrysi · 2020-12-18T23:45:24Z

@djspiewak Ok, that's fine. I can address your comments in a followup PR.

wemrysi added 3 commits December 6, 2020 10:49

Handle self-cancellation when running fibers

1c50ab5

Distinguish between cancellation and non-termination expectations in …

55d7427

…tests

Apply scalafmt

e679495

djspiewak added the CE 3 label Dec 7, 2020

djspiewak approved these changes Dec 7, 2020

View reviewed changes

wemrysi added 12 commits December 13, 2020 10:09

Update to sbt-spiewak-sonatype-0.19.2

c4fc7a6

Ensure all cases implied by the generator hierarchy are produced

d1ba17a

Enable GenSpawnLaws#raceNeverIdentityRight law

8e8cc4c

Law: Raced effects don't inherit parent mask

18e4369

Remove laws that assert masking is inherited by raced fibers

77a650f

Race with F.never is not identity when other is canceled

e104f9a

Disable raceCanceledIdentity laws

d152fd3

Update async laws to reflect callback effect is uncancelable

14bb4e2

Disable roundtrip through scala Future test

87a3d6d

Disable roundtrip through java CompletableFuture test

751d075

Disable memoize.flatten is identity test

8098ca8

Rename Root -> Main in cancellation exception

40396b5

wemrysi commented Dec 13, 2020

View reviewed changes

wemrysi added 2 commits December 13, 2020 16:58

Avoid unused warnings due to disabled tests

bd2a49a

Avoid even more unused warnings due to disabled tests

f35d430

djspiewak reviewed Dec 18, 2020

View reviewed changes

Merge branch 'series/3.x' into feature/run-fiber-cancellation

5f2b59b

mpilquist approved these changes Dec 18, 2020

View reviewed changes

djspiewak merged commit 517e9bb into typelevel:series/3.x Dec 18, 2020

This was referenced Dec 19, 2020

Attempt to restore raceNeverIdentity{Left,Right} laws #1525

Closed

Finalize, if necessary, when exiting an uncancelable region #1526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle self-cancellation when running fibers to avoid deadlocks #1484

Handle self-cancellation when running fibers to avoid deadlocks #1484

wemrysi commented Dec 6, 2020

djspiewak commented Dec 7, 2020

djspiewak left a comment

wemrysi Dec 13, 2020

djspiewak Dec 18, 2020

wemrysi Dec 18, 2020

wemrysi Dec 20, 2020

wemrysi Dec 13, 2020

djspiewak Dec 18, 2020

wemrysi commented Dec 13, 2020

djspiewak commented Dec 18, 2020

djspiewak Dec 18, 2020

djspiewak Dec 18, 2020

wemrysi Dec 18, 2020

djspiewak Dec 18, 2020

djspiewak Dec 18, 2020

djspiewak Dec 18, 2020

djspiewak Dec 18, 2020

djspiewak commented Dec 18, 2020

wemrysi commented Dec 18, 2020

djspiewak commented Dec 18, 2020

wemrysi commented Dec 18, 2020

Handle self-cancellation when running fibers to avoid deadlocks #1484

Handle self-cancellation when running fibers to avoid deadlocks #1484

Conversation

wemrysi commented Dec 6, 2020

djspiewak commented Dec 7, 2020

djspiewak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wemrysi commented Dec 13, 2020

djspiewak commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djspiewak commented Dec 18, 2020

wemrysi commented Dec 18, 2020

djspiewak commented Dec 18, 2020

wemrysi commented Dec 18, 2020