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

[API Proposal]: Add Task.Run overload that avoids capturing the ExecutionContext #80000

Closed
davidfowl opened this issue Dec 27, 2022 · 15 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading

Comments

@davidfowl
Copy link
Member

davidfowl commented Dec 27, 2022

Background and motivation

Today, suppressing ExecutionContext flow when doing Task.Run is more difficult than it needs to be and can be important when kicking off "fire and forget" work items. When running in ASP.NET Core, detaching from the incoming request context when issuing fire and forget tasks is important as there are async locals that are scoped to the incoming request. This could mess with the distributed tracing output as further http calls from the fire and forget task will show up as rooted underneath the initial request where that work originated.

e.g.

using System.Diagnostics;

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

ClassWithTimer? classWithTimer = null;

app.MapGet("/", () =>
{
    // Kick off the timer on the first request
    classWithTimer ??= new();
    return Activity.Current!.Id;
});

app.Run();

class ClassWithTimer : IDisposable
{
    private readonly PeriodicTimer _timer;

    public ClassWithTimer()
    {
        _timer = new PeriodicTimer(TimeSpan.FromSeconds(1));

        Task.Run(DoSomething);
    }

    private async Task DoSomething()
    {
        while (await _timer.WaitForNextTickAsync())
        {
            Console.WriteLine($"Activity: {Activity.Current?.Id}");
        }
    }

    public void Dispose() => _timer.Dispose();
}

The above code will print the activity id from the first request in the background timer.

PS: While answering this question, I realized how unsatisfactory all of the answers were to me 😅 :
https://www.reddit.com/r/dotnet/comments/zvm9xi/unbind_logs_from_http_request/

API Proposal

namespace System.Threading.Tasks;

[Flags]
public enum TaskCreationOptions
{
     DoNotCaptureExecutionContext = 0x80
}

[Flags]
public enum TaskContinuationOptions
{
     DoNotCaptureExecutionContext = 0x80
}

public class Task
{
    public static Task UnsafeRun(Action action);
    public static Task UnsafeRun(Action action, CancellationToken cancellationToken);
    public static Task<TResult> UnsafeRun<TResult>(Func<TResult> function);
    public static Task<TResult> UnsafeRun<TResult>(Func<TResult> function, CancellationToken cancellationToken);
    public static Task UnsafeRun(Func<Task?> function);
    public static Task UnsafeRun(Func<Task?> function, CancellationToken cancellationToken);
    public static Task<TResult> UnsafeRun<TResult>(Func<Task<TResult>?> function);
    public static Task<TResult> UnsafeRun<TResult>(Func<Task<TResult>?> function, CancellationToken cancellationToken);
}

API Usage

using System.Diagnostics;

var activity = new Activity("Activity");
activity.Start();

Task.UnsafeRun(async () =>
{
    var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
    while (await timer.WaitForNextTickAsync())
    {
        Console.WriteLine(Activity.Current?.Id);
    }
});

Console.ReadLine();

Alternative Designs

There are several:

  • Only add the TaskCreationOptions and TaskContinuationOptions enum value but not the overloads. This means that developers would need to drop down to Task.Factory.StartNew(..) to use this feature.

    using System.Diagnostics;
    
    var activity = new Activity("Activity");
    activity.Start();
    
    Task.Factory.StartNew(async state =>
    {
        var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
        while (await timer.WaitForNextTickAsync())
        {
            Console.WriteLine(Activity.Current?.Id);
        }
    },
    null,
    CancellationToken.None,
    TaskCreationOptions.DenyChildAttach | TaskCreationOptions.DoNotCaptureExecutionContext,
    TaskScheduler.Default);
    
    Console.ReadLine();
  • A boolean could be added to each of the Task.Run overloads.

  • Do nothing and tell people to use one of the other APIs to explicitly suppress the ExecutionContext flow:

    • ThreadPool.QueueUnsafeWorkItem

      using System.Diagnostics;
      
      var activity = new Activity("Activity");
      activity.Start();
      
      // Ugh, async void
      ThreadPool.UnsafeQueueUserWorkItem(async state =>
      {
          // Don't crash the process if this fails
          try
          {
              var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
              while (await timer.WaitForNextTickAsync())
              {
                  Console.WriteLine(Activity.Current?.Id);
              }
          }
          catch
          {
              // Log something
          }
      
      }, null);
      
      Console.ReadLine();
    • ExecutionContext.Suppress/Restore

      using System.Diagnostics;
      
      var activity = new Activity("Activity");
      activity.Start();
      
      ExecutionContext.SuppressFlow();
      
      Task.Run(async () =>
      {
          var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
          while (await timer.WaitForNextTickAsync())
          {
              Console.WriteLine(Activity.Current?.Id);
          }
      
      });
      
      ExecutionContext.RestoreFlow();
      
      Console.ReadLine();

Risks

A more complex API?

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 27, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 27, 2022
@ghost
Copy link

ghost commented Dec 27, 2022

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

Issue Details

Background and motivation

Today, suppressing ExecutionContext flow when doing Task.Run is more difficult than it needs to be and can be important when kicking off "fire and forget" work items. When running in ASP.NET Core, detaching from the incoming request context when issuing fire and forget tasks is important as there are async locals that are scoped to the incoming request. This could mess with the distributed tracing output as further http calls from the fire and forget task will show up as rooted underneath the initial request where that work originated.

e.g.

using System.Diagnostics;

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

ClassWithTimer? classWithTimer = null;

app.MapGet("/", () =>
{
    // Kick off the timer on the first request
    classWithTimer ??= new();
    return Activity.Current!.Id;
});

app.Run();

class ClassWithTimer : IDisposable
{
    private readonly PeriodicTimer _timer;

    public ClassWithTimer()
    {
        _timer = new PeriodicTimer(TimeSpan.FromSeconds(1));

        Task.Run(DoSomething);
    }

    private async Task DoSomething()
    {
        while (await _timer.WaitForNextTickAsync())
        {
            Console.WriteLine($"Activity: {Activity.Current?.Id}");
        }
    }

    public void Dispose() => _timer.Dispose();
}

The above code will print the activity id from the first request in the background timer.

PS: While answering this question, I realized how unsatisfactory all of the answers were to me 😅 :
https://www.reddit.com/r/dotnet/comments/zvm9xi/unbind_logs_from_http_request/

API Proposal

namespace System.Threading.Tasks;

[Flags]
public enum TaskCreationOptions
{
     DoNotCaptureExecutionContext = 0x80
}

[Flags]
public enum TaskContinuationOptions
{
     DoNotCaptureExecutionContext = 0x80
}

public class Task
{
    public static Task UnsafeRun(Action action);
    public static Task UnsafeRun(Action action, CancellationToken cancellationToken);
    public static Task<TResult> UnsafeRun<TResult>(Func<TResult> function);
    public static Task<TResult> UnsafeRun<TResult>(Func<TResult> function, CancellationToken cancellationToken);
    public static Task UnsafeRun(Func<Task?> function);
    public static Task UnsafeRun(Func<Task?> function, CancellationToken cancellationToken);
    public static Task<TResult> UnsafeRun<TResult>(Func<Task<TResult>?> function);
    public static Task<TResult> UnsafeRun<TResult>(Func<Task<TResult>?> function, CancellationToken cancellationToken);
}

API Usage

using System.Diagnostics;

var activity = new Activity("Activity");
activity.Start();

Task.UnsafeRun(async () =>
{
    var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
    while (await timer.WaitForNextTickAsync())
    {
        Console.WriteLine(Activity.Current?.Id);
    }
});

Console.ReadLine();

Alternative Designs

There are several:

  • Only add the TaskCreationOptions and TaskContinuationOptions enum value but not the overloads. This means that developers would need to drop down to Task.Factory.StartNew(..) to use this feature.
using System.Diagnostics;

var activity = new Activity("Activity");
activity.Start();

Task.Factory.StartNew(async state =>
{
    var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
    while (await timer.WaitForNextTickAsync())
    {
        Console.WriteLine(Activity.Current?.Id);
    }
},
null,
CancellationToken.None,
TaskCreationOptions.DenyChildAttach | TaskCreationOptions.DoNotCaptureExecutionContext,
TaskScheduler.Default);

Console.ReadLine();
  • A boolean could be added to each of the Task.Run overloads.
  • Do nothing and tell people to use one of the other APIs to explicitly suppress the ExecutionContext flow:
    • ThreadPool.QueueUnsafeWorkItem
    • ExecutionContext.Suppress/Restore

Risks

A more complex API?

Author: davidfowl
Assignees: -
Labels:

api-suggestion, area-System.Threading

Milestone: -

@MihaZupan
Copy link
Member

MihaZupan commented Dec 28, 2022

Re: ExecutionContext.Suppress/Restore example, you generally need even more boilerplate if your code can be called by user code you don't control as SuppressFlow will throw if the flow has already been suppressed.

Updated example using the pattern commonly used in runtime
using System.Diagnostics;

var activity = new Activity("Activity");
activity.Start();

bool restoreFlow = false;
try
{
    if (!ExecutionContext.IsFlowSuppressed())
    {
        ExecutionContext.SuppressFlow();
        restoreFlow = true;
    }
    
    Task.Run(async () =>
    {
        var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
        while (await timer.WaitForNextTickAsync())
        {
            Console.WriteLine(Activity.Current?.Id);
        }
    });
}
finally
{
    if (restoreFlow)
    {
        ExecutionContext.RestoreFlow();
    }
}

Console.ReadLine();

@davidfowl
Copy link
Member Author

@MihaZupan yep, I kept the example simple, but yes, you would need that boilerplate

@davidfowl
Copy link
Member Author

How do I get this triaged 😄

@stephentoub
Copy link
Member

How do I get this triaged

You be patient.

@davidfowl
Copy link
Member Author

UnamusedCatGIF

@ericsampson
Copy link

Re: ExecutionContext.Suppress/Restore example, you generally need even more boilerplate if your code can be called by user code you don't control as SuppressFlow will throw if the flow has already been suppressed.

I've always wondered why this couldn't be a no-op.
I suppose that would be a breaking change though 😿

@davidfowl
Copy link
Member Author

Still waiting for triage...

@stephentoub
Copy link
Member

I spoke offline with David about this.

In .NET 8, instead of:

Task.Run(DoSomething);

you can now do:

using (ExecutionContext.SuppressFlow())
    Task.Run(DoSomething);

and it'll do what you want. From my perspective, that's sufficient, in addition to the other approaches of using e.g. UnsafeQueueUserWorkItem.

If we were going to add something here, I don't think we'd do Task.UnsafeRun. Task.Run was added as the simple path for the simple cases; want to do anything more complex, like passing in object state to avoid a closure, and you fall back to using the more complete and more complicated Task.Factory.StartNew. Suppression of execution context flow counts as being "anything more complex", so we'd want to expose it there. The right way to do that would be with a new flag on TaskCreationOptions/TaskContinuationOptions. However, we're running pretty low on available bits in the Task's state flags field, which stores a bunch of information, including all of the creation options; I think we have only one left, so unless we can free up others, this would end up consuming the last slot, and anything else we wanted to do in the future that would also need a bit would likely end up being prohibitively expensive.

So, we could add:

[Flags]
public enum TaskCreationOptions
{
+     DoNotCaptureExecutionContext = ...,
}

[Flags]
public enum TaskContinuationOptions
{
+     DoNotCaptureExecutionContext = ...,
}

but I'd prefer to hold off and just rely on using (ExecutionContext.SuppressFlow()) for this particular case. It's likely a tad more expensive than doing it directly in Task, however the use cases for this I've seen all involve longer-running tasks where any such difference is likely to not add measurable impact. Other cases where we've relatively recently chosen to add UnsafeXx are in general hotter path, e.g. CancellationToken.UnsafeRegister, where the overhead could be more visible in microbenchmarks.

@davidfowl
Copy link
Member Author

Would it also make sense to make ExecutionContext.SuppressFlow not throw, if it's already suppressed? That might be the most annoying thing about using that API today.

@stephentoub
Copy link
Member

Would it also make sense to make ExecutionContext.SuppressFlow not throw, if it's already suppressed? That might be the most annoying thing about using that API today.

That's what I meant by in .NET 8 it'll do what you want. As of yesterday it no longer throws if it's already suppressed.

@davidfowl
Copy link
Member Author

Got it. Do you want me to close this or are you using this to track that the SuppressFlow changes?

@davidfowl
Copy link
Member Author

Found it #82912

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2023
@ericsampson
Copy link

Would it be worthwhile to document this new suggested pattern somewhere? Esp with the change that it no longer throws if already suppressed

@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2023
@davidfowl
Copy link
Member Author

davidfowl commented Jul 9, 2023

using System.Diagnostics;

var activity = new Activity("Activity");
activity.Start();

using (ExecutionContext.SuppressFlow())
{
  Task.Run(async () =>
  {
      var timer = new PeriodicTimer(TimeSpan.FromSeconds(1));
      while (await timer.WaitForNextTickAsync())
      {
          Console.WriteLine(Activity.Current?.Id);
      }
  
  });
}

Console.ReadLine();

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading
Projects
None yet
Development

No branches or pull requests

4 participants