Skip to content
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

Merged
merged 7 commits into from
Apr 18, 2024

Conversation

tukan
Copy link
Contributor

@tukan tukan commented Apr 15, 2024

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 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.

tukan added 3 commits April 15, 2024 20:28
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.
Copy link
Owner

@hawkw hawkw left a 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;
}

Copy link
Owner

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::forgets the guards or stuffs them someplace to hang onto them.

Copy link
Contributor Author

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

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@tukan
Copy link
Contributor Author

tukan commented Apr 15, 2024

Thank you for your comments. I will address them this week, hopefully tomorrow.

Copy link
Owner

@hawkw hawkw left a 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.

src/mpsc/tests/mpsc_blocking.rs Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) April 18, 2024 17:03
@hawkw hawkw merged commit 723c44a into hawkw:main Apr 18, 2024
25 checks passed
hawkw added a commit that referenced this pull request Apr 18, 2024
## v0.1.6 (2024-04-18)

#### Bug Fixes

*   fix senders hanging when the buffer is full (#85)
    ([723c44a](723c44a),
    closes [#83](#83))
hawkw added a commit that referenced this pull request Apr 18, 2024
## v0.1.6 (2024-04-18)

#### Bug Fixes

*   fix senders hanging when the buffer is full (#85)
    ([723c44a](723c44a),
    closes [#83](#83))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants