semaphore: make semaphore resizable #3
Conversation
|
This PR (HEAD: b0fff29) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it. Tip: You can toggle comments from me using the |
| <-ctx.Done() | ||
| return ctx.Err() | ||
| // Add doomed Acquire call to the Impossible waiters list. | ||
| waiterList = &s.impossibleWaiters |
There was a problem hiding this comment.
Wouldn't it make more sense to return sentinel error, ie ErrImpossibleWeight so that user can process it (especially since this implementation has Resize method)?
There was a problem hiding this comment.
This would be a breaking change to the behavior of the API
semaphore/semaphore.go
Outdated
| err = nil | ||
| default: | ||
| s.waiters.Remove(elem) | ||
| waiterList.Remove(elem) |
There was a problem hiding this comment.
elem may moved between waiterList and impossibleWaiters, so the elem may not be removed successfully here, cause resource leaking.
There was a problem hiding this comment.
This is a nice catch actually, here it should remove it from both lists to avoid resource leaking.
There was a problem hiding this comment.
I updated PR to fix this 👍🏻
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
This PR (HEAD: d5e4837) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it. Tip: You can toggle comments from me using the |
# Conflicts: # semaphore/semaphore.go # semaphore/semaphore_test.go
d5e4837 to
036812b
Compare
- Fixed a bug that might cause resource leak when an `acquire()` moved to the impossibleWaiters but then get its context canceled. - Upstream changes introduced a change to the API Behavior in case of an acquire() that is bigger than the size of the semaphore. Before: It was added to the queue (even though it was doomed to never get acquired) Now: It returns an error. In case of introducing Resize(), the old API behavior makes sense. because an acquire() is no longer 100% doomed to never get acquired. In case of a resize that increases the semaphore's size, the acquire can get unblocked. I reverted the API this old behavior.
|
Pull Request Updated and Synced with upstream 👍🏻 FixesChangesUpstream changes introduced a change to the API Behavior in case of an
In the case of introducing I reverted the API to this previous behavior to block and wait if the size of |
|
This PR (HEAD: 7c9f24f) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it. Tip: You can toggle comments from me using the |
|
What's the status of getting this PR merged into master? |
|
The proposal (#29721) was declined. |
|
Why was the proposal declined? :( |
Semaphores are often used to bound concurrency (e.g worker-pool) and it is sometimes preferred to be able to resize the semaphore/workerpool to bound concurrency dynamically in respond to load.
This implementation is more performant than channel-based semaphore (channel based semaphore can't be resized as channel buffer is immutable),There is another implementations of a non-channel-based semaphores that supports resizing, but I found many bugs/deadlocks using them and they're all less performant than x/sync/semaphore.
The current implementation of x/sync/semaphore can be easily extended to be resizable, without affecting performance by any means.
Github Issue: golang/go#29721
Edit:
I think I made a small mistake, I already posted the CR on Gerrit, I submitted this PR just for visibility and assumed from the repo's other PR(s) that the bot won't import it to Gerrit.
The code is identical, so I think abandoning any of the duplicate changes is fine.
Original Gerrit Review: https://go-review.googlesource.com/c/sync/+/157718