-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix issue #83: thingbuf hangs when buffer is full #85
Conversation
Previously, to determine if the buffer was full, we checked whether the head and tail were pointing to the same slot with the head one generation behind. However, this check fails if we skip slots, leading to scenarios where the head and tail point to different slots even though the buffer is full. For example, consider a buffer with 3 slots. Initially, we write to the buffer three times (gen + 0). Then, we read from slot 0 and slot 1, holding the reference from slot 1, and read from slot 2 (gen + 0). Next, we write to slot 0 (gen + 1) and read from slot 0 (gen + 1), which moves our head to slot 1 (gen + 1). Then we try to write to slot 1 (gen + 1) and skip it, so we write to slot 2 (gen + 1). Then again we write to slot 0 (gen + 2). And then we attempt to write to slot 1 but we skip and attempt to write to slot 2 (gen + 2). However, we can’t write into it because it still contains data from the previous generation (gen + 1), and our head points to slot 1 instead of slot 2. This fix ensures the buffer full condition accurately reflects the actual status of the slots, particularly when writes are skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing this! i had a few small suggestions, and i'd love to see a loom
test exercising this, but overall, this looks right to me. thank you!
@@ -696,4 +708,72 @@ mod tests { | |||
// don't panic in drop impl. | |||
core.has_dropped_slots = true; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to also add a loom
model exercising what happens when a buffer fills up due to concurrent pushes from multiple threads? we could do something where we spawn multiple threads and have each one try to push in a loop until the buffer is full, which would check that all of those threads eventually complete.
we could also exercise slot skipping by adding a thread that calls pop_ref
and either mem::forget
s the guards or stuffs them someplace to hang onto them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hawkw I need your advise here, I added a simple loom test but I find it difficult to construct a good test with read/writes under loom. Imagine we have a thread that reads from the buffer (for example, exactly three times) and we have two threads that write to the buffer until three elements are read and the buffer is full, under loom we will fail because we will reach max iterations in cases in which we attempt to write a lot and doesn't read, making us spin in "buffer is full" and not progress
Thank you for your comments. I will address them this week, hopefully tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some last questions about whether we need to add more loom
tests. But, since this PR fixes the bug, I'd like to go ahead and merge it, and we can add more tests in subsequent branches.
Fixes: #83
Previously, to determine if the buffer was full, we checked whether the head and tail were pointing to the same slot with the head one generation behind. However, this check fails if we skip slots, leading to scenarios where the
head
andtail
point to different slots even though the buffer is full.For example, consider a buffer with 3 slots. Initially, we write to the buffer three times (gen + 0). Then, we read from slot 0 and slot 1, holding the reference from slot 1, and read from slot 2 (gen + 0). Next, we write to slot 0 (gen + 1) and read from slot 0 (gen + 1), which moves our
head
to slot 1 (gen + 1). Then we try to write to slot 1 (gen + 1) and skip it, so we write to slot 2 (gen + 1). Then again we write to slot 0 (gen + 2). And then we attempt to write to slot 1 but we skip and attempt to write to slot 2 (gen + 2). However, we can’t write into it because it still contains data from the previous generation (gen + 1), and ourhead
points to slot 1 instead of slot 2.This fix ensures the buffer full condition accurately reflects the actual status of the slots, particularly when writes are skipped.