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.