Resource runs its finalizers when timed out

[fommil]

This fixes #4489 that was introduced by #4059 using an approach documented by @durban in #4489 (comment) and admitted reluctantly by @djspiewak in #4489 (comment) who suggests that there is perhaps a more general solution by rethinking Resource itself.

[djspiewak]

Can we make this so that it doesn't regress #4059? The reason timeout is implemented in terms of racePair in the first place is it's possible for the effect to complete at the exact moment of the timeout, and in that case, the value can be leaked because the finalizers on the value side have already popped. Something like this comes to mind:

IO.ref(false) flatMap { fired =>
  val program = Resource.eval(IO.sleep(100.millis).onCancel(fired.set(true)).timeout(100.millis).use_

  program.attempt flatMap {
    case Left(()) => program  // worked but we didn't reproduce the error
    case Right(_) => fired.get.flatMap(f => IO(assert(f == true)))
  }
}

The above will almost certainly fail with your fix, indicating a memory leak in the case where the timeout executes but we don't run the finalizer. (also note the above is a handwave due to the way I encoded the repetition; with a proper implementation it would loop forever, so this isn't a real unit test)

Edit: Additionally, this only fixes half of the problem. We also need to address the bug in start (described here: #4489 (comment))

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[djspiewak]

Oh, secondarily: can we retarget this at series/3.7.x please? That will allow us to release it faster. It's also fine if you need to target it at series/3.6.x. We're almost certainly going to do another release in that lineage.

[fommil]

I added a test for the simultaneous success/timeout but I can't get it to fail even with the original proposed fix 🤷🏻‍♂️ ... I'm not sure what to do to force it to be more likely to happen.

[durban]

There is something wrong with this PR, it contains completely unrelated changes from series/3.x. (I'd guess something went wrong when it was retargeted.)

[fommil]

@durban re: unrelated commits yeah that was an accident, I rebased.

BTW, "cats eye nebula" 😆 nice... is the "cats" part an intentional cats-effect joke? I have a good view of C6 from my backyard, I got eyes on it this past winter; it's really small but stunning once you find it.

[fommil]

I reverted the verbose pattern match, and added a few more tests. It would be good if these used ticked (I'll have a think about that) but it looks like only the "losing a timeout" fails on the mainline.

This PR makes all tests go green, so I still can't construct a test that lets us see the other theoretical failure case.

It's highly likely my tests are junk, though, so reviewing those would be a good first step.

[fommil]

huzzah, this latest test change seems to trigger the "leaked value" scenario. It is very non-deterministc. It fails with my change AND in the baseline. So there is always the possibility the test is junk, so please take a look at it.

Sorry for all these force pushes, I keep forgetting to scalafmt, and I wanted to include the unit tests at the start of the history.

[durban]

@fommil No, it is not an intentional Cats (Effect) reference. (I'm not even sure there was a Cats back then.) I just wanted a nice nebula for my profile picture 🤷‍♂️

[durban]

This part is strange to me. Specifically use_ing inner in the acquire part of res. Don't get me wrong, I'd expect this test to pass (and it doesn't). But the test itself is unclear to me. Was @armanbilge's minimization here not enough to reproduce the issue?

[fommil]

His minimisation is one of the three tests I added. (The other) Daniel said he expected my change to cause the value to be leaked when it completes at the same time as the timeout so I wrote this test and was surprised to find that it fails for both the mainline and my branch. The third test is a regular "timeout wins" test which seems fine for both mainline and my suggestion.

With this test I wanted a way to tell if the value has been leaked, which is why the inner Resource modifies then cleans up the ref. If you have another way of testing a leaked value that might be cleaner because there's a double use of Resource here that can be confusing.

[fommil]

I ignored the failing test, it's somewhat concerning.

[fommil]

@durban well if you ever want to take your own pic of a nebula, check out https://github.com/fommil/luddcam 😄

[fommil]

one of the tests I added is failing the windows CI build. I can take a guess at that, I bet it's because it's doing a bunch of stuff in parallel. I'll disable one more.

[fommil]

@djspiewak your test is failing even on main

  real("ensure a timed out resource runs its onCancel") {
    IO.ref(true) flatMap { fired =>
      Resource
        .eval(IO.sleep(100.millis).onCancel(fired.set(false)).timeout(100.millis))
        .use_
        .attempt
        .flatMap {
          case Left(_) => IO.unit
          case Right(_) =>
            fired.get.ifM(IO.raiseError(new Exception("didn't run the cancelation")), IO.unit)
        }
    }
  }

I've added this but flagged it .ignore. Note it passes if I use a ticker. It might benefit from replicate.

[durban]

Shouldn't the two cases be the other way around for that test? If it's a Right, then it did not time out, thus I wouldn't expect the onCancel to execute.

[fommil]

The test should probably be on both cases, but you're right. We maybe need to add some more waiting to allow the onCancel to run, I presume there's no guarantee that it runs before anything can access the value. I'd intuitively expect the cancel to run before the use completed but i guess that's not what we're seeing here.