typelevel / typelevel/cats-effect
`cancelable` may leak resources on unsuccessful cancelation
Nobody has claimed this yet.
- Dominant language
- Scala
- Stars
- 2.2k
- Forks
- 576
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 18
Description
Spinning this out of a Discord observation so that it doesn't get lost. It relates to the new cancelable combinator added in https://github.com/typelevel/cats-effect/pull/3460.
This issue is important, because it has led me to believe we cannot currently implement the non-leaking race proposed in https://github.com/typelevel/cats-effect/issues/3456.
For motivation, consider this case:
Resource.makeFull { poll =>
poll(IO.blocking(server.accept()).cancelable(server.shutdown()))
}(socket => IO(socket.close()))
There, we have a blocking call to accept a socket on a server. We can cancel it by shutting down the server. However, if the blocking call succeeds (perhaps in a race condition with shutdown()), and we do get an open socket, we really want to be sure that we close it.
To demonstrate the leak I have added a test in 84e7ef10c48a8b697dbcaf27709e74d5705caf4a.
https://github.com/typelevel/cats-effect/blob/84e7ef10c48a8b697dbcaf27709e74d5705caf4a/tests/shared/src/test/scala/cats/effect/IOSpec.scala#L1094-L1100
In this case, "cancelation" is simply causing the Deferred to complete normally. So that means the get is completing with (). If that's a resource, we don't want to lose that, so the outcome should be successful, not canceled.
It's a bit weird, but conceptually this is equivalent to a race condition where you request cancellation, but the effect in question succeeds anyway. In that case, it didn't really cancel, and if you pretend like it did, you can leak a resource.
Now, what worries me is that I don't see how we can fix this. And if we can't fix this, I don't see how we can fix https://github.com/typelevel/cats-effect/issues/3456.
So, the problem is that if cancelation occurs while this fiber is in poll(fiber.join), then it is assumed that it canceled! That means that cancelable can no longer have a successful outcome.
The problem is that until fiber completes post fiber.cancel, and we join it and find out how it completed, then we can't really say for sure whether it actually canceled or not. And if it didn't cancel, and it actually returned a successful result, we have no way to get this out of cancelable anymore. So, leak.
What we are currently lacking is a way to request cancelation, but be prepared for the fact that it might not happen. This is a common pattern: Java's Future#cancel returns a boolean indicating precisely this information. So does io_uring's cancelation mechanism.
In fact, I ran into this same problem when working on fs2-io_uring, and was forced to add a bespoke API to safely lift an async syscall into a resource. For example, a syscall opening/accepting a socket, file, or any other resource.
Not surprisingly, our Async#fromCompletableFuture suffers the exact same problem.
cf.cancel() may return false, indicating that cancelation was not possible. So then we fallback to waiting for its completion via get ... but what if this is a CompletableFuture that is asynchronously waiting for an accepted socket? Leak. So it would not be safe to use this API in a Resource.makeFull(...)(...).
Sorry for wall of problems, no solutions 😕
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the cancelation logic in kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala and the regression test in tests/shared/src/test/scala/cats/effect/IOSpec.scala around lines 1094-1100. Compare this behavior with AsyncPlatform.scala and the linked cancelable and race issues. Done means the unsuccessful-cancelation case is specified and covered without losing a successful resource-producing result.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100