GODRIVER-2533 Fix data race from NumberSessionsInProgress. #1085
Conversation
matthewdale
left a comment
There was a problem hiding this comment.
Atomic logic looks good!
mongo/integration/sessions_test.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| } |
There was a problem hiding this comment.
This select statement seems like it will block until the ctx.Done() channel is closed and will never reach the call below. To get around that, either add a default case or check if the context is done in the for loop.
E.g.
for ctx.Err() == nil {
_ = mt.Client.NumberSessionsInProgress()
}There was a problem hiding this comment.
I'm a little concerned about the busy waiting here as it occupies the CPU while waiting. Will this be an issue?
There was a problem hiding this comment.
TIL about the anti-pattern of busy-waiting, thanks 🧑🔧 . I modified the test to only run each operation 5 times and used a channel to sync the goroutines' execution as opposed to a context. Note that the test fails now only about 75% of the time when checkedOut is not atomically accessed (as opposed to closer to 95% of the time with the previous design). I think that's fine, though.
There was a problem hiding this comment.
I think short busy-wait loops in tests that are trying to exercise a specific non-deterministic behavior are probably OK, but I do think the "run N times" loops are a good way to achieve that also. I left some suggestions about possible ways to further improve the reliability of the tests in my subsequent review.
|
@matthewdale thank you for catching that the test was not actually running the functions 😮💨 . Should be fixed now; I also used a |
|
@matthewdale thanks. Removed channel, increased iteration count to 100, and added a 100µs sleep. |
GODRIVER-2533
Fixes data race in
session.Poolby switchingcheckedOutfrom anintto an atomically-accessedint64. Adds a regression test for the data race.