Skip to content

Use TaskTracket in streamer for graceful exit and avoiding flaky tests#8066

Merged
KirillLykov merged 2 commits intoanza-xyz:masterfrom
KirillLykov:klykov/use-task-tracket
Sep 26, 2025
Merged

Use TaskTracket in streamer for graceful exit and avoiding flaky tests#8066
KirillLykov merged 2 commits intoanza-xyz:masterfrom
KirillLykov:klykov/use-task-tracket

Conversation

@KirillLykov
Copy link
Copy Markdown

Problem

When streamer service is launched, it spawns task and returns handle for this task.
In tests we typically do the following:

exit.store(true, Relaxed);
task_handle.await;

//check some stats

Yet inside of this root task of streamer, it creates many other tasks to handle connections and streams, which this root task has no idea about.
Which means that when the root task has stopped we have no idea if it's child tasks have completed or not.

This leads to flaky tests. For example, in nonblocking::quic::test::test_quic_server_multiple_streams we stop the streamer and after that check stats.total_connections.load(Ordering::Relaxed). This counter counts how many task are handling at giving moment connections. It might happen that these tasks haven't finished before we do this check, so all the checks below task_handle.await are potentially flaky.

Summary of Changes

Use TaskTracker as tokio documentation suggests

@KirillLykov
Copy link
Copy Markdown
Author

KirillLykov commented Sep 16, 2025

localnet failure is due to exit timeout. I suspect that this is due to we rely on runtime to shutdown instead of proper cancelling all the tasks because now we wait for all the tasks to finish first. This is fixed in #8025
Alternatively, I could have ignored these tasks in the spawn_server but instead use it only in tests.

@KirillLykov KirillLykov force-pushed the klykov/use-task-tracket branch 2 times, most recently from d682f46 to ee7d17a Compare September 24, 2025 08:09
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (b6c922c) to head (ee7d17a).
⚠️ Report is 2082 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #8066     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         826      826             
  Lines      362561   362581     +20     
=========================================
+ Hits       301086   301100     +14     
- Misses      61475    61481      +6     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KirillLykov
Copy link
Copy Markdown
Author

@lijunwangs @alexpyattaev this PR is ready for review.

Copy link
Copy Markdown

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

The changes itself look good. Do we have any performance bench with/without this and see if any difference we should worry about as this related to the core of tracking tasks?

Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

LGTM. TaskTracker is an AtomicUsize, it should not impact perf.

@KirillLykov
Copy link
Copy Markdown
Author

The changes itself look good. Do we have any performance bench with/without this and see if any difference we should worry about as this related to the core of tracking tasks?

It is

struct TaskTrackerInner {
    state: AtomicUsize,
    on_last_exit: Notify,
}

So it is really lightweight and hardly this will lead to any observable performance effect.

Copy link
Copy Markdown

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

LGTM

@KirillLykov KirillLykov merged commit 10b07bb into anza-xyz:master Sep 26, 2025
54 checks passed
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.

4 participants