Fixed resource leak in Semaphore#withPermit and acquire(N) #403
Conversation
Codecov Report
@@ Coverage Diff @@
## master #403 +/- ##
==========================================
+ Coverage 84.19% 88.56% +4.36%
==========================================
Files 69 69
Lines 1898 1801 -97
Branches 187 188 +1
==========================================
- Hits 1598 1595 -3
+ Misses 300 206 -94 |
| case None => (Left(waiting), releaseN(n)) | ||
| case Some(m) => (Left(waiting.filterNot(_._2 eq gate)), releaseN(n - m)) | ||
| } | ||
| case Right(m) => (Right(m + n), F.unit) |
There was a problem hiding this comment.
I don't understand how this cleanup routine remembers that the acquire actually happened or not.
First of all we can have a Right(m) in that state without acquire ever happening, which means n permits will get re-added to the pool that were never acquired.
There was a problem hiding this comment.
First of all we can have a Right(m) in that state without acquire ever happening, which means n permits will get re-added to the pool that were never acquired.
No, if we have a Right(m) in state when cleanup runs, it means that acquire happened. Note this cleanup is only used when we've put a new entry in Left(waiting) for this particular acquire call. We know there was an entry in the state and we know it's no longer there, therefore we know acquire completed.
There was a problem hiding this comment.
Ah, you might be right, I was thinking of another constraint that you don't have.
|
I think this works, but you have to prove it with some tests. You can use |
* Fix issue typelevel/cats-effect#403 * Optimize acquireAsyncN * Add more tests * Fix JS tests * Optimize Semaphore.withPermit * Add credits
|
Added two tests -- prior to the fixes in this PR, the first test fails and the second test hangs. With this PR, both tests pass. |
|
Any objections to exposing |
|
I thought about it too, but I don’t think it’s user friendly. How do you explain it? |
|
What about this instead? Passing false for def withPermit[A](fa: F[A], releaseOnCompletion: Boolean): F[A]
def withPermitN[A](fa: F[A], n: Long, releaseOnCompletion: Boolean): F[A] |
|
@alexandru Bump -- I can propose the overloaded |
|
@mpilquist we can talk on a separate thread. This PR is good to go IMO. |
Addresses a leak in
withPermitdiscussed in #382. Also addresses a leak of permits whenacquire(N)is cancelled after receiving some but not all of its permits.