runtime: AsyncLocal does not survive across `yield return` statement

Description

I’m not sure this is bug or by-design. Consider the following use case of AsyncLocal<T>. Should I expect stringLocal.Value to be "Test" or null?

var stringLocal = new AsyncLocal<string>();

async IAsyncEnumerable<int> IntSequence()
{
    stringLocal.Value = "Test";
    yield return 10;
    // Now what should happen if we do not explicitly capture the execution context???
    Console.WriteLine("stringLocal.Value: {0}", stringLocal.Value);
}

From what I can observe currently, stringLocal.Value after yield return is null.

Configuration

.NET 5 / .NET 6.0.100-preview.1.21103.9

Regression?

N / A

Other information

Here is an extensive test around the persistence of AsyncLocal<T>. [.NET Fiddle]

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;

var stringLocal = new AsyncLocal<string>();

async IAsyncEnumerable<int> IntSequence(string localValue)
{
    stringLocal.Value = localValue;
    Console.WriteLine("Set up stringLocal: {0}.", localValue);
    await Task.Delay(100).ConfigureAwait(false);
    // await didn't affect AsyncLocal flow
    Console.WriteLine("stringLocal.Value: {0}", stringLocal.Value);

    using var execContext = ExecutionContext.Capture();
    Debug.Assert(execContext != null);      // i.e. ExecutionContext.IsFlowSuppressed() == false
    yield return 10;
    ExecutionContext.Restore(execContext);
    // This gives us the correct value, as we have restored execution context.
    Console.WriteLine("stringLocal.Value: {0}", stringLocal.Value);

    // Now what should happen if we do not explicitly capture the execution context???
    yield return 20;
    Console.WriteLine("stringLocal.Value: {0}", stringLocal.Value);
    yield return 30;
    Console.WriteLine("stringLocal.Value: {0}", stringLocal.Value);
}

async Task PrintSequence1()
{
    await foreach (var i in IntSequence("value 1"))
    {
        Console.WriteLine("Sequence: {0}", i);
        await Task.Delay(100);
    }
}

async Task PrintSequence2()
{
    await using var it = IntSequence("value 2").GetAsyncEnumerator();
    while (await it.MoveNextAsync())
    {
        Console.WriteLine("Sequence: {0}", it.Current);
    }
}

PrintSequence1().Wait();
Console.WriteLine();
PrintSequence2().Wait();

Here is the output

Set up stringLocal: value 1.
stringLocal.Value: value 1
Sequence: 10
stringLocal.Value: value 1
Sequence: 20
stringLocal.Value: 
Sequence: 30
stringLocal.Value: 

Set up stringLocal: value 2.
stringLocal.Value: value 2
Sequence: 10
stringLocal.Value: value 2
Sequence: 20
stringLocal.Value: 
Sequence: 30
stringLocal.Value:

About this issue

Original URL
State: open
Created 3 years ago
Reactions: 16
Comments: 22 (15 by maintainers)

Most upvoted comments

I do agree the behavior is unintuitive. I will look into what it would take to change it at the runtime level and what impact that would have.

+18

stephentoub on Feb 7, 2021

IHttpContextAcessor also will not work properly. Having such a behavior make AsyncLocal very unreliable thing to use. I would prefer a performance tradeoff to this behavior.

+10

botinko on Sep 20, 2021

I moved this out based on @stephentoub’s comment that this was by design and any change would require compiler and runtime changes - we’re not at a point in the 8.0 release where such coordinated changes would be possible - or meet the bar for something like this. @stephentoub - what are your thoughts on doing something in 9.0?

Recognize the coordination constraints at play, but if this behavior is by-design and expected to remain so through (at least) net9.0, it would be really nice to see a coordinated docs update to call this out from some of the relevant pages folks might be looking at when migrating existing code from foreach to await foreach:

austindrenski on Aug 25, 2023

@CXuesong fixing it in logging is the wrong layer. It should be done as part of the runtime. Which @stephentoub is going to look at.

davidfowl on Feb 7, 2021

Would a fix for this involve a new ExecutionContextPreservingAsyncIteratorMethodBuilder type as an alternative to AsyncIteratorMethodBuilder, and then an attribute with which the new builder type can be selected in those methods that need it? Similar to how .NET Runtime uses AsyncMethodBuilderAttribute on methods to choose between the default AsyncValueTaskMethodBuilder<TResult> and the alternative PoolingAsyncValueTaskMethodBuilder<TResult>.

KalleOlaviNiemitalo on Aug 26, 2023

IMO it’s important to fix this behavior; not just to get the behavior intuitive. Libraries like Castle Windsor and Simple Injector (two DI Containers) depend on Async Local as storage mechanism to allow their DI scopes to flow through asynchronous operations.

dotnetjunkie on Nov 20, 2021

This is a very general problem. Of course there are third party libraries which will cause unexpected issues because of this, but I managed to run into this issue with my own code. I use AsyncLocal to store my DbContext so I can conveniently access it in my async code. Now I added a method returning an IAsyncEnumerator and yield returning records and stuff stops working without any obvious, reasonable explanation.

It’s a shame this won’t work in 8.0, but please at least put it in 9.0. @KalleOlaviNiemitalo 's suggestion sounds fine to me, and shouldn’t even be a problem to get into the 8.0 release. There should be a warning in the docs, and if possible, Visual Studio should detect such cases and issue a warning.

ThomasHilbertAtCervis on Sep 28, 2023

Another compelling use case for this is distributed tracing using Activity, which also relies on AsyncLocal, e.g.:

{
  using var activity = new Activity("A");
  activity.Start();
  yield return item;
}

aelij on Jul 11, 2021

Thanks for your explanation @stephentoub, and that makes some sense.

However, aside from the MEL logging scope problem that I have bumped into, I’d like to comment on the following statement

It would also have a potential performance impact, adding an ExecutionContext.Capture() at every yield

I’ve checked the source code of ExecutionContext.Capture (or the CaptureForRestore method lying beside), it looks trivial and even might get inlined during runtime. The only place that may introduce overhead is the access to CurrentThread.

I think the point is, we should have well-defined (and consistent, unsurprising, if possible) behavior on AsycLocal. For now I fail to see any caution of usage with async stream on docs, and from the fashion MEL loggers use (or continue to use) AsyncLocal, I guess they didn’t see this coming, either.

Btw, as of now, what is the recommended pattern for me to temporarily mitigate the issue? I have to call ExecutionContext.Capture before yield return, but should I either use ExecutionContext.Restore or ExecutionContext.Run after reentry? When (or whether) should I dispose the captured ExecutionContext?

CXuesong on Feb 4, 2021

I moved this out based on @stephentoub’s comment that this was by design and any change would require compiler and runtime changes - we’re not at a point in the 8.0 release where such coordinated changes would be possible - or meet the bar for something like this. @stephentoub - what are your thoughts on doing something in 9.0?

ericstj on Aug 25, 2023

No. It’s still as it was in #47802 (comment).

@stephentoub can you please expand? There is @davidfowl comment from 2.2021 and the one you’ve linked to in your response above but not much in a way of an update on what was decided. Is this something the team is evaluating/actively looking into?

Based on @ericstj milestone bump, safe to assume no changes in the near future (year or so)?

tstojecki on Aug 25, 2023

No. It’s still as it was in https://github.com/dotnet/runtime/issues/47802#issuecomment-772700977.

stephentoub on Jul 26, 2023