Repository navigation
Conversation
ResourceConcurrent now delegates racePair to F.racePair and converts its outcomes and fibers back into Resource values. The existing allocation and finalizer tracking is shared between start and racePair, preserving resource lifetimes and cancellation behavior. The diff adds regression coverage for handled acquisition errors, finalizer timing, and cancellation. Fixes typelevel#4697
reardonj
left a comment
reardonj
left a comment
There was a problem hiding this comment.
I would have expected this implementation to look very similar to the existing race override. In fact, probably mostly replace that implementation with the new racePair. Notably, that implementation uses allocatedCase to stay inside of F as long as possible instead of lifting everything into Resource.
Build racePair the same way as race: race the allocatedCase of both sides inside F and only lift the outcomes and fibers into Resource at the end, instead of routing through a shared tracked-fiber helper. start is back to its original implementation.
|
Thanks, 092cf36 rewrites racePair to follow race: both sides race on allocatedCase and stay in F, and results are only lifted into Resource at the end. I left race on its own code for now because it cancels and finalizes the loser in the background, which racePair leaves to the caller. If you would rather have race built on top of racePair in this PR, I can do that next. |
| liftOutcome(oc, right).map(oc => Right((liftFiber(f, left), oc))) | ||
| } | ||
| .onCancel(finalizeBoth(Resource.ExitCase.Canceled)) | ||
| .map(result => (result, finalizeBoth)) |
There was a problem hiding this comment.
My only concern is that we never cancel the winner fiber when the scope closes. This could leak, but of course this is a low-level primitive so maybe that's fine. This appears to be consistent with start, which is the other way we can get a fiber of a resource. It would be nice to have a similar comment so users are aware of the danger.
There was a problem hiding this comment.
Agreed, the remaining fiber keeps running after the scope closes, same as start. 3e4aaf7 adds a comment above racePair in the style of the one on start: finalizers run at scope close if it already allocated, it releases right after allocating if it finishes late, and it leaks if it never finishes, so callers should cancel it first when that can happen.
Like start, the fiber racePair returns for the side still running is not canceled when the enclosing scope closes. Spell out what that means for its finalizers and when it can leak.
Motivation
A handled resource acquisition failure can still be reported as an unhandled error when the resource is wrapped in a timeout, producing duplicate error logs.
Resource's Concurrent instance inherited the default racePair implementation instead of delegating to the underlying effect's racePair. This bypassed the underlying effect's handling of raced fiber errors, allowing an acquisition error to be reported even when the caller handled it.
What this changes
ResourceConcurrent now delegates racePair to F.racePair and converts its outcomes and fibers back into Resource values. The existing allocation and finalizer tracking is shared between start and racePair, preserving resource lifetimes and cancellation behavior. The diff adds regression coverage for handled acquisition errors, finalizer timing, and cancellation.
How it was tested
The tests in
tests/shared/src/test/scala/cats/effect/ResourceSuite.scalafail without the fix in the cats-effect Resource change (checked by reverting the source changes). The changed test tests/shared/src/test/scala/cats/effect/ResourceSuite.scala did not run here: no test runner for it was available.Fixes #4697