Fix permit loss - #706
Fix permit loss#706ybasket wants to merge 2 commits into
Conversation
RequestSemaphore (and hence the pool) could lose permits when a waiting fiber gets cancelled with unfortunate timing. These test cases reproduce this behaviour to guide a fix and prevent regression. They're internally repeated 100 times as even with TestControl, there's some randomness as to which fiber is picked first when at a tick, several are ready.
Ports changes to the original cats-effect MiniSemaphore implementation to RequestSemaphore (commit in typelevel/cats-effect@643ba33). This fixes a bug which could lead to permit loss (aka pool leaks) if a waiting fiber get cancelled, but still gets handed a permit which is then never returned and effectively lost. The two test cases added in the previous commit now happily pass.
|
Tagging @stasimus for interest. Any @typelevel/keypool maintainers able to review this? |
reardonj
left a comment
reardonj
left a comment
There was a problem hiding this comment.
It's interesting that this MiniSemaphore copy has already drifted from the CE one, but looks like not enough for the port of this change to be invalidated.
|
I need some time for review, so far the fix itself is correct |
Yeah, the drift is real. Our It's load-bearing here though. The Lifo backing is a Accounting still checks out. Would be nice to note the delibrate difference in a comment, the scaladoc link points at v3.7.1 which no longer matches. Still need a bit more time for the full review. I might also have found a bug in the pool itself, unrelated to this PR, will write that up separately. |
| val (rest, next) = B.take(waiting) | ||
| State(rest, permits) -> next.complete(()).void | ||
| State(rest, permits) -> next.complete(()).flatMap { granted => | ||
| if (granted) F.unit else release |
There was a problem hiding this comment.
The else release here isn't exercised by the suite. I replaced it with F.unit and all 32 tests still pass, including the new ones. The matching "else release" in "cleanup" is covered, same mutation there fails all three, so it's only this half of the handshake that's unguarded.
There was a problem hiding this comment.
I checked and because of the delicate timing this one needs, I couldn't find a way to test this deterministically (both real and TestControl) without doing crazy things like providing a modified Concurrent instance. I think this acceptable though – and eventually, keypool should probably stop rolling its own Semaphore implementation, making this obsolete.
|
I accidentally found #708 |
3 participants
The bug
We've seen a ValKey connection pool that we implemented using Keypool lose connections. Investigation points to a bug in Keypool's
RequestSemaphore, a copy of an older version of cats-effect'sMiniSemaphore, that could lead to permit loss when waiters are cancelled. More precisely, a waiter that gets cancelled at the instant a permit becomes available can get it assigned even though it will never return it as it's already cancelled. The test case inRequestSemaphoreSpec.scalaillustrates it rather nicely.The fix
This has been fixed in cats-effect by the changes in typelevel/cats-effect#4648 (even though this seems to not have been the primary goal of that PR). This PR here ports the changes to
MiniSemaphoreover to Keypool and adds two test cases that reproduced the bug reliably before the fix and now are consistently successful.AI Disclaimer
AI (Fable 5.1) has been used to investigate and create the test cases, all code has been reviewed and adjusted by me, all texts are mine.
Initial investigation credits go to @christianharrington.