- Notifications
You must be signed in to change notification settings - Fork 158
semaphore: unblock waiters when the front waiter cancels #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| This PR (HEAD: 7081629) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/223418 to see it. Tip: You can toggle comments from me using the |
| Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Ian Lance Taylor: Patch Set 3: A change like this should include a test. Thanks. Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| This PR (HEAD: ee312a9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/223418 to see it. Tip: You can toggle comments from me using the |
| Message from 志强 徐: Patch Set 3:
I've uploaded a test, basically translating https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex_test.go#L43 Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| This PR (HEAD: 903461d) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/223418 to see it. Tip: You can toggle comments from me using the |
903461d to 529f392 Compare | This PR (HEAD: 529f392) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/223418 to see it. Tip: You can toggle comments from me using the |
| Message from Ian Lance Taylor: Patch Set 6: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Gobot Gobot: Patch Set 6: TryBots beginning. Status page: https://farmer.golang.org/try?commit=def2cc73 Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Gobot Gobot: Patch Set 6: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 6: (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 6: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| This PR (HEAD: 29b6ff2) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/sync/+/223418 to see it. Tip: You can toggle comments from me using the |
| Message from 志强 徐: Patch Set 6: (4 comments) I've updated with a new test 😊 Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 7: Run-TryBot+1 Code-Review+2 (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Gobot Gobot: Patch Set 7: TryBots beginning. Status page: https://farmer.golang.org/try?commit=14b8354a Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Gobot Gobot: Patch Set 7: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from 志强 徐: Patch Set 7: (2 comments) Updated, btw, about the test runner duration, does this mean the Test won't fail until 10min? Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 7:
That's correct. But since it should never fail at all, that shouldn't be a problem. 😅 Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from 志强 徐: Patch Set 7: (1 comment)
I'm ok with that then~ Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Bryan C. Mills: Patch Set 8: Run-TryBot+1 Thanks! Will submit after another TryBot pass. Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| Message from Gobot Gobot: Patch Set 8: TryBots beginning. Status page: https://farmer.golang.org/try?commit=b4c2eb2a Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
When `Release`, if the remaining tokens are not enough for the front waiter, no waiters will be notified. So if the canceled waiter is the front one, it should try to notify following waiters if there are remaining tokens. I found this bug when implementing a cancelable rwmutex based on semaphore: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex.go This bug can be verified by this test: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex_test.go#L43 Change-Id: Id8564976bd375a82c4fbc6cb08b0bb83118a346c GitHub-Last-Rev: 29b6ff2 GitHub-Pull-Request: #10 Reviewed-on: https://go-review.googlesource.com/c/sync/+/223418 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
| Message from Gobot Gobot: Patch Set 8: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/223418. |
| This PR is being closed because golang.org/cl/223418 has been merged. |
When `Release`, if the remaining tokens are not enough for the front waiter, no waiters will be notified. So if the canceled waiter is the front one, it should try to notify following waiters if there are remaining tokens. I found this bug when implementing a cancelable rwmutex based on semaphore: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex.go This bug can be verified by this test: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex_test.go#L43 Change-Id: Id8564976bd375a82c4fbc6cb08b0bb83118a346c GitHub-Last-Rev: 29b6ff2 GitHub-Pull-Request: golang#10 Reviewed-on: https://go-review.googlesource.com/c/sync/+/223418 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
When `Release`, if the remaining tokens are not enough for the front waiter, no waiters will be notified. So if the canceled waiter is the front one, it should try to notify following waiters if there are remaining tokens. I found this bug when implementing a cancelable rwmutex based on semaphore: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex.go This bug can be verified by this test: https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex_test.go#L43 Change-Id: Id8564976bd375a82c4fbc6cb08b0bb83118a346c GitHub-Last-Rev: 29b6ff26bf779d23239cfe64a395378a9e41d1fc GitHub-Pull-Request: golang/sync#10 Reviewed-on: https://go-review.googlesource.com/c/sync/+/223418 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
When
Release, if the remaining tokens are not enough for the front waiter, no waiters will be notified.So if the canceled waiter is the front one, it should try to notify following waiters if there are remaining tokens.
I found this bug when implementing a cancelable rwmutex based on semaphore:
https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex.go
This bug can be verified by this test:
https://github.com/zhiqiangxu/util/blob/master/mutex/crwmutex_test.go#L43