Skip to content

Fix semaphore wakeup count to match posted permits - #3673

Open
chenBright wants to merge 2 commits into
apache:masterfrom
chenBright:fix_bthread_sem_post
Open

chenBright wants to merge 2 commits into
apache:masterfrom
chenBright:fix_bthread_sem_post

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve #3672

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new tests can miscount queued pthread wakeups and fail despite correct semaphore behavior.

1 open issue
What changed in this PR

This PR addresses #3672 by making semaphore posts wake waiters according to newly posted permits rather than the previous semaphore count, avoiding unnecessary wakeups under contention.

Changes:

  • Pass the posted permit count to butex_wake_n.
  • Add regression tests for single-permit, batch, and zero-permit posts across different initial counts.
File Description
test/​bthread_semaphore_unittest.cpp Adds wake-count regression tests using bthread and pthread waiters.
src/​bthread/​semaphore.cpp Corrects the wake limit and removes the unused previous count.

Review effort: Balanced

Comment thread test/bthread_semaphore_unittest.cpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused fix matches the wakeup API contract, and the regression tests cover the affected behavior with no unresolved findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bthread_sem_post passes the old semaphore count to butex_wake_n, causing thundering herd

2 participants