Skip to content

fix: https://github.com/triton-inference-server/server/issues/8537 - #435

Open
aleksn7 wants to merge 1 commit into
triton-inference-server:mainfrom
aleksn7:issue-8537
Open

fix: https://github.com/triton-inference-server/server/issues/8537#435
aleksn7 wants to merge 1 commit into
triton-inference-server:mainfrom
aleksn7:issue-8537

Conversation

@aleksn7

@aleksn7aleksn7 commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Fix the race between the done callback async_event_future_done_callback and the background_futures_.attr("add")

Pull request for the issue described here:
triton-inference-server/server#8537

@whoisj

Copy link
Copy Markdown
Contributor

Thank you for your contribution. @aleksn7, have you signed a CLA? If not, please see Contributing / CLA.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a race condition in the Python backend stub’s async coroutine handling for decoupled execution, ensuring background futures are tracked before their completion callback can run.

Changes:

  • Reorders background_futures_.add() to occur before add_done_callback() in Stub::RunCoroutine.
  • Adds an in-code comment referencing the upstream issue for context.

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

Comment threadsrc/pb_stub.cc
Comment on lines 943 to 945
py_future.attr("add_done_callback")(
py::module_::import("c_python_backend_utils")
.attr("async_event_future_done_callback"));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This should never happen.

@aleksn7

aleksn7 commented May 12, 2026

Copy link
Copy Markdown
ContributorAuthor

Thank you for your contribution. @aleksn7, have you signed a CLA? If not, please see Contributing / CLA.

Hello! I've just signed the CLA

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

LGTM. CLA received. @yinggeh can you take a quick look as well? Thanks.

@yinggeh

Copy link
Copy Markdown
Contributor

Pipeline ID: 51138619

@yinggeh

Copy link
Copy Markdown
Contributor

LGTM. However, this change seems to introduce test failures. It might be noise but still unclear. We will merge the PR once it's clear. cc @whoisj

@aleksn7

Copy link
Copy Markdown
ContributorAuthor

LGTM. However, this change seems to introduce test failures. It might be noise but still unclear. We will merge the PR once it's clear. cc @whoisj

Hi! Do you have any updates on this?

@whoisj

Copy link
Copy Markdown
Contributor

@aleksn7, apologies for slow reply as we've been off work for the past five days. I'm looking into the test failures. Looks like they might be caused by something unrelated to this PR, but they do make validating the PR difficult. We'll return to this PR once the test failures have been resolved. Again, apologies for the delay.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@aleksn7@whoisj@yinggeh