feat: port context-aware cond to gobox #325
Open
+483
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've been using this for a few months in polaroid (non-generic version), so I figured it's reasonable at this point to port it to GoBox,
because Jesse was worried about extra allocations, I aded a couple of benchmarks which also revealed a race that could result in an NPE: ``` BenchmarkCond BenchmarkCond/one_broadcasts;_one_wait BenchmarkCond/one_broadcasts;_one_wait-10 100 11171022 ns/op 1.170 ms_corrected/op 489 B/op 9 allocs/op BenchmarkCond/one_broadcasts;_many_waiters BenchmarkCond/one_broadcasts;_many_waiters-10 100 11239043 ns/op 1.230 ms_corrected/op 2023 B/op 37 allocs/op PASS ```
|
because Jesse was worried about extra allocations, I aded a couple of benchmarks which also revealed a race that could result in an NPE: |
jaredallard
reviewed
Jul 21, 2023
jaredallard
approved these changes
Jul 21, 2023
jkinkead
approved these changes
Aug 7, 2023
| // a condition, and releasing the lock before waiting for a state change if the condition is not met. | ||
| type Cond struct { | ||
| pointer atomic.Pointer[chan struct{}] | ||
| Mu sync.Mutex |
Comment on lines
+31
to
+33
| t := make(chan struct{}) | ||
| c.pointer.CompareAndSwap(nil, &t) | ||
| return t |
There was a problem hiding this comment.
This doesn't handle the race where two threads enter this if block at the same time. You should return the value of c.pointer.Load() here (although technically only needed if CompareAndSwap returns false).
Correct verbose version:
Suggested change
| t := make(chan struct{}) | |
| c.pointer.CompareAndSwap(nil, &t) | |
| return t | |
| t := make(chan struct{}) | |
| if c.pointer.CompareAndSwap(nil, &t) { | |
| return t | |
| } | |
| return c.pointer.Load() |
There was a problem hiding this comment.
Probably should close t in my example before line 35.
| } | ||
| } | ||
|
|
||
| // WaitForCondition acquires Cond's lock, then checks if the condition is true. If the condition is not true, |
There was a problem hiding this comment.
Clarify what the lock actually is (and use the receiver name):
Suggested change
| // WaitForCondition acquires Cond's lock, then checks if the condition is true. If the condition is not true, | |
| // WaitForCondition acquires c.Mu, then checks if the condition is true. If the condition is not true, |
| // This method encapsulates the instructions in sync.Cond.Wait: | ||
| // | ||
| // """ | ||
| // Because c.L is not locked while Wait is waiting, the caller |
There was a problem hiding this comment.
Update L to Mu throughout the example?
There was a problem hiding this comment.
yah, thanks!
Co-authored-by: Jesse Kinkead <jesse.kinkead@outreach.io>
Co-authored-by: Jesse Kinkead <jesse.kinkead@outreach.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been using this for a few months in polaroid (non-generic version), so I figured it's reasonable at this point to port it to GoBox,
What this PR does / why we need it
Jira ID
I didn't make a ticket for this because I'm really just trying to be a good citizen here, this isn't part of feature work or anything.
[XX-XX]
Notes for your reviewers