Fix unlawful instances

[johnspade]

Port to ZIO 2 and apply the following fixes by @neko-kai from the master branch:

• Fix #541 #509 respect finalizer in Async#async implementation #542
• Always convert non-interrupt failures to Outcome.Errored. Define standard Concurrent/Temporal instances only for Throwable error. Define generic Concurrent/Temporal as operating on Cause[E] errors to be able to wrap non-interrupt errors to Outcome.Errored. #543
• Fix #503 implement MonadCancel#canceled by sending an external interrupt to current fiber via Fiber.unsafeCurrentFiber #544
• Outcome conversion and test fixes #549

[CLAassistant]

All committers have signed the CLA.

[johnspade]

@neko-kai please help with the failing tests 🙏

[neko-kai]

@johnspade
Hi, I fixed one of the failing tests, CatsInteropSpec, but there are still failures that surface when genOfRace/genOfParallel or genCancel generators are enabled. I'm not sure if these are caused by more uncovered behavior or bugs in ZIO 2.
I would suggest to investigate errors with enabled genCancel and disabled genRace & genParallel

[johnspade]

@neko-kai
These tests are failing with enabled genCancel and disabled genRace & genParallel:

• Temporal[Task].temporal.onCancel associates over uncancelable boundary
• GenSpawn[IO[Int, _], Cause[Int]].concurrent.onCancel associates over uncancelable boundary
• MonadCancel[IO[In t, _], Cause[Int]].monadCancel.onCancel associates over uncancelable boundary
• Async[Task].async.onCancel associates over uncancelable boundary

It looks like we have to fix #562.

[neko-kai]

@johnspade
Yes, I've seen them fail with genCancel & disabled genRace/genParallel. There are no laws that check for issues in #562, so I doubt that's it. By investigating I mean using debugger & prints (and possibly overriding law code to insert debugging or rewriting the law in pure ZIO to inspect it closely) - to obtain understanding of what actually fails exactly. I suspect ZIO 2 has bugs which cause these failures, because as far as I see you copied the fixes in ZIO1 correctly.

[johnspade]

The failing case for the "monadCancel.onCancel associates over uncancelable boundary" law looks like this in pure ZIO:

def canceled = {
  def loopUntilInterrupted: UIO[Unit] =
    ZIO.descriptorWith(d => if (d.interrupters.isEmpty) ZIO.yieldNow *> loopUntilInterrupted else ZIO.unit)

  for {
    _ <- ZIO.withFiberRuntime[Any, Nothing, Unit]((thisFiber, _) => thisFiber.interruptAsFork(thisFiber.id))
    _ <- loopUntilInterrupted
  } yield ()
}

ZIO.unit.onExit(_ => canceled *> ZIO.never)

This code hangs and fails the test, but works fine with ZIO.interrupt. It means that this Cats code won't work correctly:

F.onCancel(F.uncancelable(_ => fa), F.canceled *> F.never)

thisFiber.interruptAsFork can't interrupt itself inside onExit because it's an uninterruptible region. @neko-kai any ideas?

[neko-kai]

@johnspade
You probably found the problem. This code shouldn't hang on ZIO1. Check this line https://github.com/zio/interop-cats/pull/595/files#diff-e27c0f2c26e6b28ceb1612ce7fe2ea034311be23ed842e07678687c6b4c6459fR280 - the else ZIO.unit clause is not actually dead code - with ZIO1 interrupters will be filled even when we're in an uninterruptible region, so canceled will return Unit instead of hanging when uninterruptible.

If this hangs on ZIO2 that means it works differently now - we may need to find another way to reliably check for external interrupters (in uninterruptible region). /cc @adamgfraser

Or we could just give up and return Unit immediately if we're in an uninterruptible region.

Edit: Actually, looking at the examples again, they should hang anyway because of *> never. canceled shouldn't interrupt when inside an uninterruptible region, so the hanging is entirely correct, as long as it happens in never, not in canceled.

[adamgfraser]

Don't we just need to use ZIO.interrupt?

object Example extends ZIOAppDefault {

  val awaitInterruption: UIO[Any] =
    ZIO.descriptorWith(d => if (d.interrupters.nonEmpty) ZIO.interrupt *> ZIO.never else awaitInterruption)

  val run =
    for {
      promise <- Promise.make[Nothing, Unit]
      fiber   <- ZIO.unit.onExit(_ => promise.succeed(()) *> awaitInterruption).forkDaemon
      _       <- promise.await
      exit    <- fiber.interrupt
      _       <- Console.printLine(exit)
    } yield ()
}

[neko-kai]

Cats canceled shouldn't interrupt immediately when in uninterruptible region, but return Unit and delay interruption until the uninterruptible region is over, so that's correct that we don't use ZIO.interrupt

[adamgfraser]

Okay, then I think we want something like:

object Example extends ZIOAppDefault {

  val canceled: UIO[Any] =
    ZIO.fiberIdWith { fiberId =>
      FiberRef.interruptedCause.update(_ ++ Cause.interrupt(fiberId))
    }

  val run =
    for {
      fiber <- ZIO.unit.onExit(_ => canceled *> ZIO.debug("Still going")).forkDaemon
      exit  <- fiber.await
      _     <- Console.printLine(exit)
    } yield ()
}

[johnspade]

Actually, looking at the examples again, they should hang anyway because of *> never.

Yeah, sorry, I got confused, this is working as intended.

[johnspade]

We can ignore results like Succeeded(None), it means that IO under tests did not produce a result (IO.never, for example)

[neko-kai]

@johnspade We can't ignore it. If both lhs and rhs hang then it's fine, but not if one of the sides finishes and the other hangs. And case _ => true here ignores everything, not just Succeeded(None)

[johnspade]

Well, cats-effect uses similar relaxed Eq for law testing

[neko-kai]

@johnspade

Well, cats-effect uses similar relaxed Eq for law testing

I would really love to know how you came up with that, but cats-effect tests use the eqIOA instance defined in TestInstances - https://github.com/typelevel/cats-effect/blob/series/3.x/tests/shared/src/test/scala/cats/effect/IOSpec.scala#L1580 - which uses the Eq[Outcome[Option, Throwable, A]] instance - i.e. out1 eqv out2 here, that was used before - which checks all the branches not just success.

  implicit def eq[F[_], E: Eq, A](implicit FA: Eq[F[A]]): Eq[Outcome[F, E, A]] =
    Eq instance {
      case (Canceled(), Canceled()) => true
      case (Errored(left), Errored(right)) => left === right
      case (Succeeded(left), Succeeded(right)) => left === right
      case _ => false
    }

[johnspade]

@neko-kai Please take a look if everything is okay in this PR, I can fix it in the separate PR

[johnspade]

@adamgfraser I've tried your implementation for canceled but unfortunately some tests are failing (I didn't check why)

[adamgfraser]

@johnspade Okay. Is this still an issue? Can we isolate what the expected behavior is that we need to support?

[johnspade]

@adamgfraser No, everything is working fine now (at least according to law tests). I was wrong and my example should hang with Cats.

Cats canceled shouldn't interrupt immediately when in uninterruptible region, but return Unit and delay interruption until the uninterruptible region is over

This is the expected behavior.

[adamgfraser]

@johnspade Okay, great!

[neko-kai]

@adamgfraser @jdegoes @johnspade
The expected behavior is for both sides of the equation to hang, not just one of them. This line - f13c6c0#diff-b8ed2c048152009ccfdae3f8027be0a596b699ca0785aa61dc72000427289372R138 - doctors the test results and makes nearly everything pass. The laws are still not fixed. I would revert the merge in this case tbh

[johnspade]

F.uncancelable(_ => F.onCancel(fa, fin)) <-> F.onCancel(F.uncancelable(_ => fa), fin)

When the genCancel generator is used for the left side and the genNever generator is used for the right side, they can't be equal. The left side will not hang in this case.

[neko-kai]

@johnspade
The same generator is used on both sides, since fa is the result of generator, so this can't happen

[johnspade]

@neko-kai I've observed such behavior in my debugging, I guess generators are randomly chosen on each fa evaluation

[neko-kai]

@johnspade In that case no tests would ever pass at all, because e.g. genSuccess and genFail would never be equal and also there would be different values inside genSuccess. This is also structurally impossible because we only have Gen[IO[A]] not IO[Gen[A]] - the output IO is determined once the generator is run.

Also, the tests also fail when using catsConversionGenerator in GenIOInteropCats - since that converts from cats IO generator - that works with cats IO laws, it shouldn't have the non-determinism you describe.

Also it works on ZIO1 branch which has about the same generators and eq instances.

What you could have observed is genParallel/genRace running effects in indeterminate order, however the fa is guaranteed to be the same exact instance and not to have random generators inside it that would make it different when run.

[adamgfraser]

@neko-kai I'm fine with reverting this until we are all comfortable with the changes here since it seems like we are still figuring this out.

@johnspade I will take a look myself but did my suggestion fix the issue with this test? I know you said it caused issues but I think we need to understand what those issues are.

[johnspade]

Okay, let's revert and reopen it

[johnspade]

@adamgfraser No, this test (monadCancel.onCancel associates over uncancelable boundary) is still failing with your suggestions

[adamgfraser]

@johnspade Okay let's take a look at that.