Skip to content

#204: Use default scheduler for worker tasks #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
atanzu merged 2 commits into from
Jun 10, 2026
+45 −1
Merged

#204: Use default scheduler for worker tasks #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
647b2ae
#204: Use default scheduler for worker tasks
George-Shaw Jun 5, 2026
15a8966
Add regression test for #204
duncanista Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/StatsdClient/Worker/AsynchronousWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public AsynchronousWorker(
_optionalExceptionHandler = optionalExceptionHandler;
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));

@duncanista duncanista Jun 4, 2026

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.

@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:

  1. 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.
  2. 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.

@George-Shaw George-Shaw Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, my intent was to keep all existing behaviour except for inheriting the caller's scheduler.
  2. 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.

@George-Shaw George-Shaw Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 duncanista Jun 4, 2026

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.

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

@George-Shaw George-Shaw Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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!

@duncanista duncanista Jun 5, 2026

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.

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.

@George-Shaw George-Shaw Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

Expand Down
44 changes: 44 additions & 0 deletions tests/StatsdClient.Tests/Worker/AsynchronousWorkerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,33 @@ public void Flush()
}
}

// Regression test for https://github.com/DataDog/dogstatsd-csharp-client/issues/204.
// The worker must not inherit the caller's TaskScheduler; otherwise a long-running
// Dequeue() loop can end up pinned to a UI SynchronizationContext and freeze the app.
[Test]
[Timeout(5000)]
public void WorkerTasksDoNotInheritCallerScheduler()
{
const int workerThreadCount = 3;
var trackingScheduler = new TrackingTaskScheduler();

var outerTask = Task.Factory.StartNew(
() => CreateWorker(workerThreadCount: workerThreadCount),
CancellationToken.None,
TaskCreationOptions.None,
trackingScheduler);

Assert.IsTrue(outerTask.Wait(TimeSpan.FromSeconds(3)));

// The only task queued on the tracking scheduler should be the outer task itself.
// If AsynchronousWorker inherits TaskScheduler.Current, the worker tasks would be
// queued here too, bringing the count up to 1 + workerThreadCount.
Assert.AreEqual(
1,
trackingScheduler.QueueCount,
"AsynchronousWorker leaked worker tasks onto the caller's TaskScheduler (issue #204).");
}

#if NETFRAMEWORK
/// <summary>
/// This test can only fail when run on the .NET Framework in 64-bit release build using RyuJIT.
Expand Down Expand Up @@ -127,6 +154,23 @@ private AsynchronousWorker<int> CreateWorker(int workerThreadCount = 2)
return worker;
}

private sealed class TrackingTaskScheduler : TaskScheduler
{
private int _queueCount;

public int QueueCount => Volatile.Read(ref _queueCount);

protected override void QueueTask(Task task)
{
Interlocked.Increment(ref _queueCount);
ThreadPool.UnsafeQueueUserWorkItem(_ => TryExecuteTask(task), null);
}

protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued) => false;

protected override IEnumerable<Task> GetScheduledTasks() => Array.Empty<Task>();
}

#if NETFRAMEWORK
private class AppDomainDelegate : MarshalByRefObject
{
Expand Down
Loading