Fix #4627 resource early cancelation

[reardonj]

Adds uncancelable wrapper to evaluation of resource so that cancelation can only occur at reasonable points.

---

This fix does a slightly more involved poll wrapping of the calls @durban identified as fix targets in #4627 . Passes both the law tests and a new test for the bug.

I'm a little suspicious of the poll wrapping the allocate case as it aught to break the tailrec, but allocatedCase already polls its continues inside it's Allocated case, so I guess it isn't any worse than existing code.

EDIT: oh, I see now, the Allocate case was just suspending the whole thing in bracketFull and returning anyways. Tail recursion only happens in Bind and Pure.

[durban]

I now realize my suggested "obvious fix" is completely incorrect. But I don't think this one's correct either. For example with this branch, this test fails:

  real("eval - uncancelable continuation") {
    def res(d: Deferred[IO, Unit]) = Resource.make(IO.pure(42))(_ => IO.unit).flatMap { _ =>
      Resource.eval(IO.uncancelable { _ => d.complete(()).as(99) })
    }
    val t = for {
      d <- IO.deferred[Unit]
      ctr <- IO.ref(0)
      fib <- IO.uncancelable { poll =>
        poll(res(d).allocatedCase).flatMap { _ =>
          ctr.update(_ + 1)
        }
      }.start
      _ <- d.get
      _ <- fib.cancel
      c <- ctr.get
      _ <- IO { assertEquals(c, 1) }
    } yield ()
    t.replicateA_(1000)
  }

Unless I'm missing something, this is just the original problem with some extra steps, so it probably shouldn't fail

[reardonj]

I was suspicious of the poll around the bracketFull and continue in allocatedCase.

I golfed your failing test down to a deterministic one that fails consistently, then narrowed the uses of poll more. I had to do an odd thing (poll(F.unit) >> continue(…)) in the allocatedCase implementation to add a cancelation boundary without polling the whole continuation. This gets the new test to pass without violating any of the laws, except for:

  tickedProperty("combineK - behave like orElse when underlying effect does") {
    implicit ticker =>
      forAll { (r1: Resource[IO, Int], r2: Resource[IO, Int]) =>
        val lhs = r1.orElse(r2)
        val rhs = r1 <+> r2

        assertEqv(lhs, rhs)
      }
  }

It does not consistently fail, but fails with this seed at least:

  override def scalaCheckInitialSeed = "EpTk-jCEjNXrCuelFnCg7QRFmJK5gqhF6DEW9EUaFtF="

orElse and <+> end up calling completely different code paths (handleErrorWith vs combineK), which feels a little suspicious if they're supposed to behave the same, but that's as far as I'm getting today.

[reardonj]

Poking at this a bit more, in Resource, combineK has a bespoke implementation which propagates the final exit case to both inner resource finalizers, while orElse is derived from handleErrorWith, which does not propagate the final exit case…so it's not surprising to me that the combineK test fails, though it doesn't fail specifically because of that (the structure ends up different as well).

Should handleErrorWith match the combineK behavior? That feels odd, since the first resource obviously failed. Or maybe we need a bespoke orElse? I'm not really clear on the justification for why the exit case propagation should work one way or the other.

Poking @armanbilge in case you have any insight, since you were modifying the propagation behavior a few years ago to propagate the same exit case to both sides for combineK: #3307

[reardonj]

Hmm, orElse is only syntax anyways, so can't really override it.

[stasimus]

I reran the deleted test with your pinned seed on both sides: passes on `3845296dc6`, fails on `461008f4f2`. So it isn't a pre-existing failure that the seed happened to surface, something on this branch changed it. Which matches your July 8 note that the exit case propagation isn't what actually breaks it.

My guess is the masking asymmetry rather than the finalizers. combineK is built on fold, and this branch wraps the whole of fold in uncancelable. orElse goes through attempt into allocatedCase, where:

- poll(continue(head(b), tail, rel2))
+ (poll(F.unit) >> continue(head(b), tail, rel2))

That turns the continuation from cancelable into masked, with a single cancelation point in front of it. So the two sides now open their cancelation windows in different places, and in a ticked test that's enough to make them disagree.

Quick way to confirm: put that one line back to poll(continue(...)) and rerun with the seed. If the property goes green then that's the cause, and the real question is whether the narrower poll is right (it's what makes the new eval test pass) or whether the outer uncancelable on fold is too coarse for combineK.

[stasimus]

Ran it, my guess was wrong. Putting poll(continue(...)) back doesn't help, the property still falsifies after 13 tests.

It's the fold hunk. Revert just that, the uncancelable wrapper around the interpreter, and keep the allocatedCase changes, and everything goes green: ResourceSuite 216/216 with the test and your seed restored, testsJVM/test 3018/3018. Both new eval tests still pass without it, so that hunk doesn't look necessary for #4627. JVM only, I didn't try JS or Native.

[reardonj]

It's the fold hunk. Revert just that

So, I didn't actually add a test to exercise fold. I guess I should because I would expect it to fail the same way if I reverted it, but I'll have to get this code back in my head. I still don't think the test is valid anyways since handleErrorWith and combineK do have different behavior ignoring my changes.

[stasimus]

I digged a bit more,

whether the test is valid, the Eq the property uses says it itself:

cats-effect/kernel-testkit/shared/src/main/scala/cats/effect/kernel/testkit/TestInstances.scala

Lines 68 to 78 in 3845296

	/**
	* Defines equality for a `Resource`. Two resources are deemed equivalent if they allocate an
	* equivalent resource. Cleanup, which is run purely for effect, is not considered.
	*/
	implicit def eqResource[F[_], A](
	implicit E: Eq[F[A]],
	F: MonadCancel[F, Throwable]): Eq[Resource[F, A]] =
	new Eq[Resource[F, A]] {
	def eqv(x: Resource[F, A], y: Resource[F, A]): Boolean =
	E.eqv(x.use(F.pure), y.use(F.pure))
	}

Two resources are deemed equivalent if they allocate an equivalent resource. Cleanup, which is run purely for effect, is not considered.

It's E.eqv(x.use(F.pure), y.use(F.pure)), so the comparison is the use outcome and nothing else. The exit case passed to a finalizer never reaches it, which means the combineK vs handleErrorWith difference can't be what fails here. What the Eq does see is Succeeded/Errored/Canceled, and wrapping fold in uncancelable changes exactly that?

[reardonj]

@stasimus , sure, but I want to make sure fold actually isn't bugged without the wrapping.

[stasimus]

I think it does consistently fail, it just needs more cases than the default. ScalaCheck's budget is 100 and munit doesn't override it, while on your branch the default seed falsifies at 152.

With seed gEuK2d5wMaG6hAem1iPvdbemcyKX2vZY0Qp6nVhrACO= at case 152:

orElse   = Succeeded(None)
combineK = Canceled()

Succeeded(None) is unsafeRun's initial value, so that side never completed. The same pair doesn't diverge on base, and base survives 50k cases across two seeds. Might be worth a look before the test goes.

[stasimus]

@stasimus , sure, but I want to make sure fold actually isn't bugged without the wrapping.

i'll check all 3 variants, base, head and head minus fold hunk

[stasimus]

I had a go at that. Built three trees, base, your head, and your head with just the fold hunk reverted, and ran four shapes that all interpret through use, so fold's Eval branch:

A  Resource.eval(IO.uncancelable(_ => IO.sleep(100.millis))).timeout(10.millis).use_
B  make(...).flatMap(_ => Resource.eval(IO.uncancelable(_ => IO.canceled))).use_
C  same resource, allocatedCase
D  same resource, use(_ => IO.unit)

            base                    head              head minus fold hunk
A   Left(TimeoutException)       Right(())               Right(())
B   Canceled() ctr=0            Canceled() ctr=0        Canceled() ctr=0
C   Canceled() ctr=0            Succeeded ctr=1         Succeeded ctr=1
D   Canceled() ctr=0            Canceled() ctr=0        Canceled() ctr=0

Head and head-minus-hunk agree on all four, so I couldn't get fold to misbehave without the wrapping. Not proof, I only tried shapes I could think of.

What did surprise me is B and D. allocatedCase now runs the continuation, but the same resource through use still doesn't, so those two paths disagree on your branch where they agreed on base. Is that intended?

[reardonj]

@stasimus , can you share the full code? I'm unclear where ctr comes from.

[stasimus]

@reardonj please look into reardonj#3

ctr is just a Ref in a helper, same shape as your eval - uncancelable continuation test:

IO.uncancelable(poll => poll(allocate).flatMap(_ => ctr.update(_ + 1)))
where allocate is use_, allocatedCase or use(f) depending on the shape, and it reports the join outcome alongside the count. The other file is the deleted property with the failing seed pinned, so it "falsifies" at 152 and prints both sides

[reardonj]

What did surprise me is B and D. allocatedCase now runs the continuation, but the same resource through use still doesn't, so those two paths disagree on your branch where they agreed on base. Is that intended?

If you had the use inside the poll, that makes sense. There is a cancelation point right inside the use, while there should be none after allocatedCase completes (the bug in question).

---

I had assumed that the fix must also happen in fold, since use_ was involved in @durban's example in the bug, so I was perplexed as to how we could remove the uncancelable from fold, but looking over it all again, the use of .timeout involves allocatedCase, so we do only need a fix there! fold's use of bracketFull is enough there.

I took the liberty of doing a squash and rebase to get the build more stable and so my incorrect removal of the law test doesn't end up in history