Fix Thread pool priority alternation

[eduardo-vp]

The thread pool is currently not alternating between high-priority and normal-priority work items.

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR fixes a bug in the thread pool's priority alternation mechanism. The issue was that the flag controlling whether to dispatch normal-priority work first was only being toggled when certain conditions were met, rather than on every dequeue attempt. This prevented proper alternation and could lead to starvation of normal-priority work items.

Changes:

• Moved the toggle of _dispatchNormalPriorityWorkFirst flag outside the conditional block to ensure it happens on every call to DequeueWithPriorityAlternation

[jkotas]

Is this a recent regression?

[eduardo-vp]

It seems that the bug was introduced when the time-sensitive work item queue was removed and just the high-priority queue was used instead in #64834.

[agocke]

If this is in 10 and we need a backport we'll need a writeup of the full issue for triage.

[VSadov]

Considering that it has not been noticed so far (since net7.0 ?), I wonder if the difference has a material impact at all or whether we have a way to measure it.
Does the fix make a difference on TE or on some other benchmarks?

[mangod9]

yeah would be good to know if this is fixing a perf or a functional issue?

[eduardo-vp]

I see that this code for the time-sensitive work item queue was introduced in #61930 as a fix for #61804. Looks like in .NET 6 without this queue it was possible to end up with starved worker threads with the timer callbacks not being able to run.

However, I checked that the issue doesn't repro in .NET 8, 9 and 10. In that case, rather than enabling the priority alternation, I guess that logic can be removed as well since we haven't seen starvation issues as described in #61804 and naturally high priority work items are preferred.

[eduardo-vp]

Since the priority alternation was introduced to solve #61804 but even without it that's no longer an issue, I think it should be safe to remove it as we were not using it anyway for the past releases.

[jkotas]

cc @stephentoub

[mangod9]

so assume this doesnt have any perf implications correct?

[agocke]

Can we run te/other benchmarks before merging?