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.
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
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!
| // don't panic in drop impl. | ||
| core.has_dropped_slots = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
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::forgets the guards or stuffs them someplace to hang onto them.
There was a problem hiding this comment.
@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. |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
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.
2 participants
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
headandtailpoint 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
headto 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 ourheadpoints 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.