Fix unlawful instances

[johnspade]

Continue pushing #595

[adamgfraser]

Can we create a reproducer of the failing test case using just ZIO?

[johnspade]

I think it should look like this but both sides are hanging for me here:

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.uninterruptibleMask(_ => canceled.onExit(_ => ZIO.never.unit)) <-> ZIO.uninterruptibleMask(_ => canceled).onExit(_ => ZIO.never.unit)

[adamgfraser]

@neko-kai I'm looking at this law and having a little trouble figuring out how it is supposed to work:

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

Per "uncancelable eliminates onCancel" it appears that the onCancel within an uncancelable block is a no-op so the left hand side can be reduced to F.uncancelable(_ => fa) and we can say that fin will never be run.

However, in the right hand side uncancelable only prevents cancelation from being observed within the uncancelable block, so my expectation would have been that if fa was canceled, cancellation would be observed immediately after uncancelable completed execution and then fin would be run.

What am I missing here?

[neko-kai]

@adamgfraser
This law seems to intentionally defy equational reasoning, there's an explanatory comment:

  /*
   * NB: This is effectively in violation of the monad laws, since
   * we consider finalizers to associate over the boundary here, but
   * we do NOT consider them to right-associate over map or flatMap.
   * This simply stems from the fact that cancelation is fundamentally
   * uncomposable, and it's better to pick a semantic for uncancelable
   * which allows regional composition, since this avoids "gaps" in
   * otherwise-safe code.
   *
   * The argument is that cancelation is a *hint* not a mandate. This
   * holds for self-cancelation just as much as external-cancelation.
   * Thus, laws about where the cancelation is visible are always going
   * to be a bit off.
   */

Avoiding 'gaps' here seems to mean that an uncancelable region should grant uncancelability to its outer error handlers as well - so avoiding getting interruption immediately after F.uncancelable ends.

I don't know why ZIO1 passes this, on a glance I couldn't find anything that specifically enables this behavior.

It seems like the problem is indeed that we're running fin on RHS, so there's a "gap" and we get interrupted, because we're getting this error:

Canceled() was not equal to Succeeded(None)
failing seed for temporal.onCancel associates over uncancelable boundary is 0aW1CPkqTDS3zfIlbHxnS2LtXto7X4DV-dD4ctszzRO=

So LHS is interrupted, but RHS is hanging. We can only get interrupted if fa=cancel, otherwise fin can never run. Since RHS hanged, fin is probably never. @johnspade Sorry if that's what you meant by getting cancel and never at the same time, I read that as you meant that you got fa=cancel on LHS and simultaneously fa=never on RHS.

However, we also hang without genNever, with only genRace or genParallel enabled. Seems like that could happen if we generate something like F.both(F.cancel, F.cancel), so we run into this clause in race:

   *   8. If the race is masked and is canceled because one or both
   *   participants canceled, the fiber will block indefinitely.

I'm not sure how critical is this law - or why ZIO1 passes it - but it's possible to ignore it by subclassing AsyncTests and removing it from the list.

[johnspade]

The funny thing is that I can reproduce it with cats.effect.IO, IO.uncancelable(_ => IO.canceled.onCancel(IO.canceled *> IO.never)).unsafeRunSync() cancels instead of hanging. I'm not sure cats-effect even tests this particular case for IO, they disable the racePair generator "since it reifies nondeterminism, which cannot be law-tested".

[neko-kai]

@johnspade
That's correct, it should cancel, not hang. By the time we get the cancel signal - outside of IO.uncancelable - we're already outside of the onCancel handler's region. They only remove racePair from nested generations, so you can still get F.racePair as the outermost expression in generated IO, so this case is tested with race – but you don't need that since F.cancel and F.never are enough for this test.

For this law both the RHS and the LHS should cancel, not hang. fin should never execute. It tests that in F.onCancel(F.uncancelable(_ => fa) <>, fin) there's no interruptible region right after F.uncancelable, at <>. That's not true for ZIO2 - there is an interruptible region, seems like it should be there for ZIO1 too, but for some reason there isn't - or it's non-deterministic, maybe due to the fast-path in interpreter - and it passes.

[johnspade]

This cancelation stuff is way over my head :(

@neko-kai

For this law both the RHS and the LHS should cancel, not hang

But IO.canceled.onCancel(IO.uncancelable(_ => IO.canceled *> IO.never)).unsafeRunSync() hangs for me, does it break this law?

[adamgfraser]

@neko-kai I did see that comment. 😃

@johnspade I think these are the ones you want to test, which both are canceled:

object Example extends IOApp {

  val left =
    IO.uncancelable(_ => IO.canceled.onCancel(IO.never))

  val right =
    IO.uncancelable(_ => IO.canceled).onCancel(IO.never)

  def run(args: List[String]): IO[ExitCode] =
    left *> IO(ExitCode.Success)
}

However, I think this is basically because cancelation is only checked on flatMap. For example with the following modification right will not terminate:

object Example extends IOApp {

  val left =
    IO.uncancelable(_ => IO.canceled.onCancel(IO.never))

  val right =
    (IO.uncancelable(_ => IO.canceled) *> IO.unit).onCancel(IO.never)

  def run(args: List[String]): IO[ExitCode] =
    left *> IO(ExitCode.Success)
}

This feels like it will be a difficult property to support precisely because it doesn't support equational reasoning and arguably relies on an implementation detail of interruption only being checked for between certain operations.

It seems like at this point we do feel like these changes are correct as far as they go and make the tests more robust by testing more generated values. So perhaps we should disable this test, merge this PR, and open a new issue to explore how, if at all, this behavior can be supported?

[neko-kai]

As I've said, I'm not sure how important this law is - but it doesn't seem that important or strong enough to me, so I think it's ok to disable the test and merge.

[johnspade]

Okay, cool, I'll create an issue and disable the test

[adamgfraser]

@johnspade Thank you for your work on this! 🙏

[johnspade]

Thank you for your help @adamgfraser & @neko-kai!