Skip to content
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.

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

System.Threading.Tasks.Task.WhenAny<TResult>(IEnumerable) inconsistently does not use current scheduler for tasks #87481

Closed
cretz opened this issue Jun 13, 2023 · 15 comments

Comments

@cretz
Copy link

cretz commented Jun 13, 2023

Description

There are 6 overloads for Task.WhenAny:

  1. WhenAny(IEnumerable<Task>)
  2. WhenAny(Task[])
  3. WhenAny(Task, Task)
  4. WhenAny<TResult>(IEnumerable<Task<TResult>>)
  5. WhenAny<TResult>(Task<TResult>[])
  6. WhenAny<TResult>(Task<TResult>, Task<TResult>)

It appears that 1, 2, 3, and 6 properly use the current task scheduler only and never the default scheduler, but that 4 and 5 use ContinueWith on the default scheduler to schedule the casting. To make this more confusing, if you call 5 with an array of 2 it uses a different scheduler than calling 5 with an array of 3.

For my use case, I have to prevent things from getting on the default scheduler for determinism (and write an analyzer for this). This is easy to know when it's ContinueWith or Run or Start or whatever which all have predictable scheduler choice (be it default or current). But for WhenAny it's not easily predictable.

See

return intermediate.ContinueWith(Task<TResult>.TaskWhenAnyCast.Value, default,
TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.DenyChildAttach, TaskScheduler.Default);
and
return intermediate.ContinueWith(Task<TResult>.TaskWhenAnyCast.Value, default,
TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.DenyChildAttach, TaskScheduler.Default);
.

Reproduction Steps

With a custom scheduler as current, call await Task.WhenAny(new[] { Task.FromResult("a"), Task.FromResult("b") }) and confirm the default scheduler is not used for continuation but await Task.WhenAny(new[] { Task.FromResult("a"), Task.FromResult("b"), Task.FromResult("c") }) does.

I caught this via my TplEventSource listener.

Expected behavior

Any .NET task (or task factory) calls should clearly use one scheduler or another, not mix them. And the documentation should say as much. Maybe the continue-with can use the first task's scheduler?

(or I am wrong, maybe this is the expected behavior, and I should never expect to know that default scheduler will not be used internally)

Actual behavior

Depending on which WhenAny call you make, you may end up running something on the default scheduler.

Regression?

No response

Known Workarounds

I could stop using event source listeners to catch people putting tasks on the wrong scheduler, but it leaves users open to accidents.

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 13, 2023
@ghost
Copy link

ghost commented Jun 13, 2023

Tagging subscribers to this area: @dotnet/area-system-threading-tasks
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

There are 6 overloads for Task.WhenAny:

  1. WhenAny(IEnumerable<Task>)
  2. WhenAny(Task[])
  3. WhenAny(Task, Task)
  4. WhenAny<TResult>(IEnumerable<Task<TResult>>)
  5. WhenAny<TResult>(Task<TResult>[])
  6. WhenAny<TResult>(Task<TResult>, Task<TResult>)

It appears that 1, 2, 3, and 6 properly use the current task scheduler only and never the default scheduler, but that 4 and 5 use ContinueWith on the default scheduler to schedule the casting. To make this more confusing, if you call 5 with an array of 2 it uses a different scheduler than calling 5 with an array of 3.

For my use case, I have to prevent things from getting on the default scheduler for determinism (and write an analyzer for this). This is easy to know when it's ContinueWith or Run or Start or whatever which all have predictable scheduler choice (be it default or current). But for WhenAny it's not easily predictable.

See

return intermediate.ContinueWith(Task<TResult>.TaskWhenAnyCast.Value, default,
TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.DenyChildAttach, TaskScheduler.Default);
and
return intermediate.ContinueWith(Task<TResult>.TaskWhenAnyCast.Value, default,
TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.DenyChildAttach, TaskScheduler.Default);
.

Reproduction Steps

With a custom scheduler as current, call await Task.WhenAny(new[] { Task.FromResult("a"), Task.FromResult("b") }) and confirm the default scheduler is not used for continuation but await Task.WhenAny(new[] { Task.FromResult("a"), Task.FromResult("b"), Task.FromResult("c") }) does.

I caught this via my TplEventSource listener.

Expected behavior

Any .NET task (or task factory) calls should clearly use one scheduler or another, not mix them. And the documentation should say as much. Maybe the continue-with can use the first task's scheduler?

(or I am wrong, maybe this is the expected behavior, and I should never expect to know that default scheduler will not be used internally)

Actual behavior

Depending on which WhenAny call you make, you may end up running something on the default scheduler.

Regression?

No response

Known Workarounds

I could stop using event source listeners to catch people putting tasks on the wrong scheduler, but it leaves users open to accidents.

Configuration

No response

Other information

No response

Author: cretz
Assignees: -
Labels:

area-System.Threading.Tasks, untriaged

Milestone: -

@stephentoub
Copy link
Member

(or I am wrong, maybe this is the expected behavior, and I should never expect to know that default scheduler will not be used internally)

It's always possible that libraries might use the default scheduler internally. That's exactly what .ConfigureAwait(false) leads to, and practically every single await anywhere throughout the core libraries (and most libraries, by guidance) uses .ConfigureAwait(false).

@cretz
Copy link
Author

cretz commented Jun 13, 2023

Right, I tell users not use ConfigureAwait (or to use ConfigureAwait(true)) and to disable CA2007 for deterministic code. I will be writing an analyzer to catch that early, but it's a bit more difficult in unpredictable situations like what is here.

@stephentoub
Copy link
Member

stephentoub commented Jun 13, 2023

The behavior / implementation that's in place is by design.

@cretz
Copy link
Author

cretz commented Jun 13, 2023

That's unfortunate. I was hoping that I could easily teach people how to avoid the thread pool without saying "do not use WhenAny with generic result type with an enumerable or more than two tasks in the array". I see much of the rest of the design for Task methods is clearer about when which scheduler is used.

Would y'all be willing to change the generic WhenAny to use TaskFactory.CommonCWAnyLogic (i.e. make that generic) like the non-generic WhenAny does instead of a ContinueWith that uses the default scheduler?

.NET docs in many cases say whether a call will use the current scheduler or default scheduler. If it is (unfortunately) by design that it may be half and half, can a remark be added or similar?

@stephentoub
Copy link
Member

.NET docs in many cases say whether a call will use the current scheduler or default scheduler. If it is (unfortunately) by design that it may be half and half, can a remark be added or similar?

Effectively every XxAsync method across all of .NET could use the default scheduler / thread pool under the covers.

Would y'all be willing to change the generic WhenAny to use TaskFactory.CommonCWAnyLogic (i.e. make that generic) like the non-generic WhenAny does instead of a ContinueWith that uses the default scheduler?

Every one of the cited Task.WhenAny methods can result in the default scheduler / thread pool being used. The use of ContinueWith in the cited method isn't changing that; it's not even forcibly queueing anything... that continuation is created as ExecuteSynchronously, so by default nothing will be queued... it'll only result in a work item being queued to the default scheduler if something prevented it from running synchronously, such as the antecedent specifying RunContinuationsAsynchronously, or being too deep on the stack, or some such thing.

@cretz
Copy link
Author

cretz commented Jun 13, 2023

being too deep on the stack

I noticed this one too while reading the code for UnwrapPromise.Invoke

Hrmm. Can you think of any way I can tell developers not to use the thread pool but still use the task framework? I see durable functions here and here basically just says "please don't" without much help. I actually see now they a synchronization context to try to help here whereas I use an event source listener to try to do the same. Maybe this is worth a more general discussion of configurable task management in .NET or is it more likely that ship has already sailed?

For now I have made my own admittedly-less-performant WhenAny workaround that is async and awaits the non-generic one and casts the result instead of making a non-async version using continue with. I have asked users to use my version instead.

@stephentoub
Copy link
Member

Can you think of any way I can tell developers not to use the thread pool but still use the task framework?

Nope. Effectively every async method anywhere in the core libraries might end up using the thread pool. It's an inherent part of the runtime. It'd be like trying to avoid ever touching the GC. :)

@cretz
Copy link
Author

cretz commented Jun 27, 2023

Yet avoiding the thread pool is exactly what the Microsoft Azure team (here and here) and the Microsoft Orleans team (here) are asking people to do. And for the most part the rules are simple but not here.

The Orleans docs even say:

await, TaskFactory.StartNew (see below), Task.ContinueWith, Task.WhenAny, Task.WhenAll, Task.Delay all respect the current task scheduler

But that's not true for Task.WhenAny as this issue now makes clear. I may notify them.

Not sure anything must change, but I think predictability of which scheduler will be used by default on different Task calls is a reasonable request.

@stephentoub
Copy link
Member

Yet avoiding the thread pool is exactly what the Microsoft Azure team (here and here) and the Microsoft Orleans team (here) are asking people to do.

That's about where user code runs. It's non-sensical, for example, to say that Task.Delay "respects" the TaskScheduler, since it uses timers that inherently run system code on the thread pool.

@cretz
Copy link
Author

cretz commented Jun 27, 2023

Agreed, Orleans got Delay wrong in their docs. Azure says "never use [...] Task.Delay" as do I at https://github.com/temporalio/sdk-dotnet/#workflow-logic-constraints. But yes, this is all about user code avoiding the thread pool for determinism reasons in all 3 projects. (and of course we're very excited about the new time provider API recently merged)

@stephentoub
Copy link
Member

I understand the desire, but I don't see anything actionable. Saying implementations can't use the thread pool as an implementation detail would inhibit practically every async method in the core libraries and beyond.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 27, 2023
@cretz
Copy link
Author

cretz commented Jun 27, 2023

I was just hoping that this one inconsistency in this one method in this one situation could be fixed to use the current scheduler the way the rest of this method uses the current scheduler.

@stephentoub
Copy link
Member

stephentoub commented Jun 29, 2023

I don't see it as an inconsistency. The others don't explicitly target the current scheduler; they're just not creating any delegate-backed tasks to be executed. The generic WhenAny was creating one as an implementation detail in order to handle a particular required cast, however that continuation was also ExecuteSynchronously, so unless it was too deep on the stack (in which case lots of things can end up being queued), it wouldn't actually be queueing work or behaving any differently, other than tracing you happening to be monitoring that are suggesting things are happening that aren't (yes, the task is associated with the default scheduler, but that doesn't mean it's forcibly incurring any kind of thread or context switch).

That said, avoiding that extra task is valuable for perf reasons, so I opened a PR to do so: #88154. This is not motivated by avoiding using the thread pool, which generally wouldn't have been used, anyway.

@cretz
Copy link
Author

cretz commented Jun 29, 2023

Thanks! This really helps the consistency of Task API scheduler predictability (even if not consistent across methods, consistent itself). Very appreciated.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants