typelevel / typelevel/cats-effect

`cancelable` may leak resources on unsuccessful cancelation

Open
#3,474 13 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

:beetle: bug :book: laws
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.

https://github.com/typelevel/cats-effect/blob/7a5311b18bfb6c553a15227492be5c6f80d9d941/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala#L264-L269

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.

https://github.com/typelevel/cats-effect/blob/a9eac6cf8685a43bb45db2d4ce35a387a7ec8c52/kernel/jvm/src/main/scala/cats/effect/kernel/AsyncPlatform.scala#L52-L56

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.