Skip to content

Fix flaky StreamPoolTest.ConcurrentThreadsGetDistinctStreams - #23820

Merged
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
vuule:fix-concurrent-stream-pool-test
Aug 26, 2026
Merged

Fix flaky StreamPoolTest.ConcurrentThreadsGetDistinctStreams#23820
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
vuule:fix-concurrent-stream-pool-test

Conversation

@vuule

@vuule vuule commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Description

ConcurrentThreadsGetDistinctStreams relied on a latch to keep its two threads overlapping, but the latch only guarantees that both threads have started, not that both have acquired a stream pool.

Each thread now takes its pool before waiting on the latch, so both pools are live at the same time.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

The latch only guaranteed that both threads had started, not that both had
acquired a pool. Since a pool is created on first use and returned to the free
list when its thread exits, a thread that was starved past the other's exit
adopted the retired pool and saw the same streams.
@vuule

vuule commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test f686451

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 25, 2026
@vuule vuule added bug Something isn't working tests Unit testing for project non-breaking Non-breaking change labels Aug 25, 2026
@NVIDIA NVIDIA deleted a comment from copy-pr-bot Bot Aug 25, 2026
@vuule
vuule marked this pull request as ready for review August 25, 2026 22:51
@vuule
vuule requested a review from a team as a code owner August 25, 2026 22:51
@vuule
vuule requested review from mythrocks and vyasr August 25, 2026 22:51
@vuule vuule changed the title Fix flaky StreamPoolTest.ConcurrentThreadsGetDistinctStreams Fix flaky StreamPoolTest.ConcurrentThreadsGetDistinctStreams Aug 25, 2026

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

Nice one.

@mythrocks

Copy link
Copy Markdown
Contributor

This can't have an effect on the spark-rapids-jni CI pipeline. The error there is because cudf-spark-jni needs to account for rapidsai/rmm#2527. I'm working on fixing that.

Please ignore that failure.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8411fe0b-c3af-447d-9df3-899e1cc07bfd

📥 Commits

Reviewing files that changed from the base of the PR and between 8f09dc1 and f686451.

📒 Files selected for processing (1)
  • cpp/tests/utilities_tests/stream_pool_tests.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Improved concurrency test coverage to verify simultaneous stream-pool acquisition across threads.
    • Added clearer comments describing stream-pool creation and reuse after thread termination.

Walkthrough

The concurrency test now acquires and records each thread’s initial streams before the latch. Comments describe first-use pool creation and retired-pool adoption after thread exit.

Changes

Stream-pool concurrency test

Layer / File(s) Summary
Pre-latch stream acquisition
cpp/tests/utilities_tests/stream_pool_tests.cpp
Each thread requests and records its initial streams before waiting on the latch. Comments document pool creation and retired-pool adoption.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to f6864

This is a localized test synchronization fix with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: vyasr, mythrocks

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the flaky test and states that the pull request fixes it. It matches the main change.
Description check ✅ Passed The description accurately explains the latch race and the change that ensures both threads acquire pools before proceeding.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@vuule

vuule commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit a45a521 into NVIDIA:main Aug 26, 2026
164 of 165 checks passed
@vuule
vuule deleted the fix-concurrent-stream-pool-test branch August 26, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants