#204: Use default scheduler for worker tasks by George-Shaw · Pull Request #205 · DataDog/dogstatsd-csharp-client

George-Shaw · 2025-05-08T16:38:41Z

Fix for issue #204.

AsynchronousWorker called Task.Factory.StartNew without a TaskScheduler argument. The default behaviour inherits the task scheduler from the parent task. If the parent task was explicitly scheduled in a particular synchronisation context (e.g. the main thread), the worker tasks would use that same context. This was incorrect - worker tasks should always be in background threads.

To correct this, I am explicitly specifying TaskScheduler.Default for scheduling the worker tasks, so they always run on thread pool threads.

This is the most minimal fix for the issue. It would probably be better to use Task.Run instead of Task.Factory.StartNew.

cit-pr-commenter · 2025-05-08T19:03:09Z

PR Security Update

All commits in this PR up to and including 625c149 have been reviewed and marked safe by SDLC security. For any questions, please reach out to #ci-for-external-contributors-collab on Slack.

Copilot

Pull request overview

Fixes issue #204 by ensuring AsynchronousWorker does not inherit the caller’s TaskScheduler when starting its long-lived worker tasks, preventing worker loops from being scheduled onto UI/main-thread synchronization contexts.

Changes:

Explicitly schedules worker tasks using TaskScheduler.Default (instead of inheriting the ambient scheduler).

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

duncanista · 2026-06-04T15:07:40Z

            for (int i = 0; i < workerThreadCount; ++i)
            {
-                _workers.Add(Task.Factory.StartNew(() => Dequeue(), TaskCreationOptions.LongRunning));
+                _workers.Add(Task.Factory.StartNew(() => Dequeue(), CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default));


@George-Shaw thanks for the fix. quick question on this codex comment: the description says workers will "always run on thread pool threads", but TaskCreationOptions.LongRunning with TaskScheduler.Default actually spins up a dedicated background thread rather than using the pool. the fix itself is still correct for the issue you're solving (keeping work off a caller-supplied scheduler like a UI SynchronizationContext), it's just the wording that's a bit off.

a couple things we wanted to check with you:

was the intent to keep the existing dedicated-thread-per-worker behavior and just stop inheriting the caller's scheduler? that's what the code does today and what this patch preserves, which seems right to us.

would you be open to rewording the PR description so it says something like "always run on background threads, off the caller's scheduler" instead of "thread pool threads"? that way the description matches the runtime behavior and avoids confusion for future readers.

also, separately, the line in your description suggesting Task.Run would be better: agreed it reads more cleanly, but Task.Run doesn't accept TaskCreationOptions.LongRunning, so swapping to it would change behavior (workers would run on pool threads instead of dedicated ones, with potential pool-starvation risk at higher workerThreadCount). probably worth either dropping that line or moving it to a "future work" note so it doesn't read as part of this PR's scope.

Yes, my intent was to keep all existing behaviour except for inheriting the caller's scheduler.

I've updated the PR description with your corrections. You are quite right about the effect LongRunning has on the default scheduler and that it cannot be used with Task.Run.

I'll also update the commit message in a little bit.

Commit message now updated to remove the reference to "thread pool threads".

I also rebased the commit on the latest master while I was at it.

duncanista · 2026-06-04T15:07:49Z

            for (int i = 0; i < workerThreadCount; ++i)
            {
-                _workers.Add(Task.Factory.StartNew(() => Dequeue(), TaskCreationOptions.LongRunning));
+                _workers.Add(Task.Factory.StartNew(() => Dequeue(), CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default));


@George-Shaw on this codex comment about a regression test: worth adding, since this is the kind of bug that's easy to silently reintroduce later (someone "simplifies" the StartNew call back to the shorter overload and nothing visibly breaks until a UI app hits it again).

a minimal version could be: construct a custom TaskScheduler that records every call to QueueTask, run new AsynchronousWorker(...) inside a task pinned to that scheduler via Task.Factory.StartNew(..., yourScheduler), then assert the recorded queue never received the worker tasks. that exercises the exact inheritance path from issue #204 without needing a real SynchronizationContext or UI host.

do you have bandwidth to add it to this PR? if not, no worries, we'll take care of it on our end. just let us know.

Agreed, a regression test would be a good idea here. The test you describe should do the job.

Unfortunately, I've recently come back from holiday to a large backlog of work, so I'm unlikely to have time to do this anytime soon. Sorry for the inconvenience!

no worries at all, thanks for letting us know. just pushed the regression test as commit d6d0b11 on top of your branch (tests/StatsdClient.Tests/Worker/AsynchronousWorkerTests.cs::WorkerTasksDoNotInheritCallerScheduler).

it builds a TrackingTaskScheduler that counts QueueTask invocations, then constructs AsynchronousWorker from a task pinned to that scheduler. with the fix the queue count is 1 (just the outer task); without the fix it's 1 + workerThreadCount. confirmed locally that it passes on the current branch and fails when the TaskScheduler.Default argument is removed.

also wanted to say thanks for updating the description and commit message, the new wording reads great. ptal when you have a sec and let us know if anything looks off about the test, otherwise we'll move to get this merged.

Test looks good to me. I'm impressed with how quickly you did that. I guess that's an advantage to already being familiar with the test suite...

duncanista

thanks for the fix. left a couple of comments inline on the codex notes (PR description wording, and a regression test). nothing blocking, just want to align on the wording and figure out who picks up the test before this merges.

atanzu · 2026-06-08T13:33:10Z

@George-Shaw could you please sign your commit? Unfortunately due to the repo rules I can't merge it as-is. Please have a look at this.

AsynchronousWorker called Task.Factory.StartNew without a TaskScheduler argument. The default behaviour inherits the task scheduler from the parent task. If the parent task was explicitly scheduled in a particular synchronisation context (e.g. the main thread), the worker tasks would use that same context. This was incorrect - worker tasks should always be in background threads. To correct this, I am explicitly specifying TaskScheduler.Default for scheduling the worker tasks, so they always run on background threads, off the caller's scheduler.

Verifies that AsynchronousWorker does not inherit a caller-supplied TaskScheduler. The test pins construction to a TaskScheduler that counts QueueTask invocations; if the worker tasks were to inherit the caller's scheduler, they would be queued there too. With the fix in place, only the outer construction task is queued.

George-Shaw · 2026-06-09T13:11:32Z

@atanzu: I've re-pushed the commits with signatures. I also signed @duncanista's commit that added the regression test, since that was on top of my commit. (If the repo rules require the original author to sign rather than the committer, @duncanista will need to re-sign that commit.)

atanzu · 2026-06-10T11:14:16Z

@George-Shaw @duncanista thank you a lot for your contribution!

atanzu requested a review from Copilot June 4, 2026 12:31

Copilot started reviewing on behalf of atanzu June 4, 2026 12:31 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

duncanista reviewed Jun 4, 2026

View reviewed changes

duncanista force-pushed the gshaw/issue-204 branch from 625c149 to b13d451 Compare June 4, 2026 15:13

duncanista requested a review from a team as a code owner June 4, 2026 15:13

George-Shaw force-pushed the gshaw/issue-204 branch from b13d451 to bb9e4ac Compare June 5, 2026 17:41

duncanista approved these changes Jun 5, 2026

View reviewed changes

atanzu approved these changes Jun 8, 2026

View reviewed changes

George-Shaw and others added 2 commits June 9, 2026 14:03

George-Shaw force-pushed the gshaw/issue-204 branch from d6d0b11 to 15a8966 Compare June 9, 2026 13:06

gh-worker-dd-devflow-36fce6 Bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Jun 10, 2026

atanzu merged commit 43706d1 into DataDog:master Jun 10, 2026
28 checks passed

gh-worker-dd-devflow-36fce6 Bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Jun 10, 2026

Conversation

George-Shaw commented May 8, 2025

Uh oh!

cit-pr-commenter Bot commented May 8, 2025

PR Security Update

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

duncanista Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

George-Shaw Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

George-Shaw Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

duncanista Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

George-Shaw Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

duncanista Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

George-Shaw Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

duncanista left a comment

Choose a reason for hiding this comment

Uh oh!

atanzu commented Jun 8, 2026

Uh oh!

George-Shaw commented Jun 9, 2026

Uh oh!

Uh oh!

atanzu commented Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants