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

[reardonj]

A case where this could matter has been noted in a keypool issue. It has a PR, but it would be much nicer if Resource just worked.

[reardonj]

@durban , do you have any time to take another look at this?

[durban]

Sorry, I couldn't follow the discussion, and reading it now, it's unclear to me, why fold doesn't need fixing. In particular, why is this: fa.flatMap(a => continue(Resource.pure(a), stack)) not problematic in fold?

[reardonj]

Sorry, I couldn't follow the discussion, and reading it now, it's unclear to me, why fold doesn't need fixing. In particular, why is this: fa.flatMap(a => continue(Resource.pure(a), stack)) not problematic in fold?

I haven't found a way to break it in fold. The allocatedCase situation is problematic because it is possible the very last action to be an uncancelable Eval that completes, but the result is lost (the "eval - uncancelable continuation" test). I don't see how to break it in the places where fold is used, as they all have a cancellation point after the eval anyway:

• fold: If only a bare Eval is being folded, the next loop calls onOutput in the Pure branch, which can cancel anyways. There is no material difference between canceling at the flatMap or the onOutput.

• allocatedCase: If only a bare Eval is being folded, the next loop call returns the pair with the release. In this case, arguably we should always return without cancelling.

---

Looking closer at the keypool code, I don't think it is actually related to this issue at all. It's Eval is cancelable.

[durban]

Okay, so, is Resource#flatMap supposed to introduce a cancellation point (during acquire)? That is, if a and b are resources with an uncancellable acquire, can a *> b be cancelled after the acquire of a, but before the acquire of b?

Intuitively I would've said no. However, it is clearly yes for allocatedCase (with or without this PR). But then again, reading the code it seems to me, the answer is no for fold!

Interestingly, removing the poll(F.unit) >> part on your branch makes some law tests fail. (I'm sure you've noticed this.) What's weird is that the failing laws have nothing to do with cancellation, so I'm confused...

[durban]

Oh, sorry, I've misread fold; bracketFull introduces a cancellation point there too.

(I still don't understand the law failures.)

[reardonj]

Okay, so, is Resource#flatMap supposed to introduce a cancellation point (during acquire)? That is, if a and b are resources with an uncancellable acquire, can a *> b be cancelled after the acquire of a, but before the acquire of b?

It can happen in IO, so why wouldn't it be the case here? IO.uncancelable(a) *> IO.uncancelable(b) still has 2 cancelation points (start and *>).

I am reading the preservation of interruptability (which probably should have a terminology update in the docs? That's not CE3 terms) as being specifically about Resource.eval. As in, If you life an uncancelable effect into Resource via eval, that Resource must also be uncancelable. An a convenient way to demonstrate that is via timeout, as its contract forbids timing out the uncancelable side. But once you compose that resource with others, cancelation is on the table. The point of how Resources compose is that you should be able to safely cancel inbetween steps and the finalizers will do the right thing.

Interestingly, removing the poll(F.unit) >> part on your branch makes some law tests fail. (I'm sure you've noticed this.) What's weird is that the failing laws have nothing to do with cancellation, so I'm confused...

I get 3 failing tests:

• async.fiber join is guaranteeCase which appears to involve cancellation:

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

  Lines 133 to 134 in de84c01

  	def fiberJoinIsGuaranteeCase[A](fa0: F[A], f: Outcome[F, E, A] => F[Unit]) = {
  	// the semantics of cancelation create boundary conditions we must avoid

• async.race never non-canceled identity (left) which sounds cancelation related

• ditto for right identity

I also don't really understand the law tests insofar that I don't know how they go about reducing forms to try making them the same.

[durban]

Sorry, what I should've wrote is that (as far as I can see) the law tests never attempt to cancel the 2 sides. So any difference in cancellability should be unobservable. And yet, they fail...

[reardonj]

The tests do take arbitrary fa params. In an earlier version, I had law tests only failing sometimes, so maybe some of the fas that scalacheck produces involve cancelation.

[durban]

Yeah, you're right, indeed the IO generator generates self-cancelling things.