Skip to content

Bugfix: bthread_interrupt was scheduled to the wrong tag group#3351

Open
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_interrupt_tag
Open

Bugfix: bthread_interrupt was scheduled to the wrong tag group#3351
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_interrupt_tag

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve #3349

Problem Summary:

bthread_stop() calls bthread_interrupt(tid), which uses the default
argument BTHREAD_TAG_DEFAULT = 0. When a non-worker thread (e.g. the
main thread during Server::~Server() -> Join()) stops a bthread with tag > 0,
TaskGroup::interrupt() takes the c->choose_one_group(tag)->ready_to_run_remote(m)
path with tag == 0. As a result, the interrupted bthread is re-scheduled onto a
worker of tag 0 instead of its own tag group, breaking tag isolation.

What is changed and the side effects?

Changed:

The tag of a bthread is already recorded in its TaskMeta
(m->attr.tag), so passing the tag explicitly is unnecessary and
error-prone. Instead of forwarding the (default) tag argument, the code
now reads the tag from the target TaskMeta:

  • bthread_interrupt(bthread_t, bthread_tag_t): the tag parameter is
    now ignored (kept only for API/ABI compatibility) and the call
    delegates to TaskGroup::interrupt(tid, control).
  • TaskGroup::interrupt(): the tag parameter is removed; the target
    group is chosen via c->choose_one_group(m->attr.tag).
  • butex.cpp: the redundant ButexBthreadWaiter::tag field is removed;
    butex_wake, butex_wake_n, butex_wake_except, butex_requeue and
    erase_from_butex now obtain the tag from task_meta->attr.tag.

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.

Pull request overview

Fixes incorrect tag-group scheduling when interrupting/stopping bthreads from non-worker threads (e.g., Server::~Server()->Join()), aiming to preserve tag isolation by selecting the target TaskGroup based on the interrupted bthread’s tag.

Changes:

  • Removes the tag argument from TaskGroup::interrupt() and switches remote re-scheduling to use the target task’s recorded tag.
  • Keeps bthread_interrupt(bthread_t, bthread_tag_t) for API/ABI compatibility but ignores the tag parameter internally.
  • Removes ButexBthreadWaiter::tag and routes butex wake/requeue scheduling via task_meta->attr.tag.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/bthread/task_group.h Updates TaskGroup::interrupt signature to remove the tag parameter.
src/bthread/task_group.cpp Chooses remote task group for interrupted sleepers based on the target task’s tag.
src/bthread/butex.cpp Removes stored waiter tag and attempts to schedule wakeups using task_meta->attr.tag.
src/bthread/bthread.cpp Keeps bthread_interrupt signature but forwards to the new TaskGroup::interrupt overload.
Comments suppressed due to low confidence (1)

src/bthread/task_group.cpp:1096

  • The change routes interruption/wakeup scheduling based on a tag, but there’s no unit test covering the reported regression scenario (non-worker thread stops a sleeping bthread with tag>0 and it must be rescheduled within the same tag group). Adding a test would help prevent future regressions in tag isolation for bthread_stop()/bthread_interrupt() and the usleep/butex wake paths.
int TaskGroup::interrupt(bthread_t tid, TaskControl* c) {
    // Consume current_waiter in the TaskMeta, wake it up then set it back.
    ButexWaiter* w = NULL;
    uint64_t sleep_id = 0;
    int rc = interrupt_and_consume_waiters(tid, &w, &sleep_id);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/bthread/task_group.cpp
Comment thread src/bthread/butex.cpp
Comment thread src/bthread/butex.cpp
Comment thread src/bthread/butex.cpp
Comment thread src/bthread/butex.cpp
Comment thread src/bthread/butex.cpp
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_stop函数中bthread_interrupt的调度问题

2 participants