dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.7k stars 4.59k forks source link

AsyncLocal is behaving differently after upgraging from DNX #17340

Closed ivaylokenov closed 4 years ago

ivaylokenov commented 8 years ago

Steps to reproduce

I have the following class:

namespace MyTested.Mvc.Internal.Caching
{
    using System.Linq;
    using System.Collections.Generic;
#if NET451
    using System.Runtime.Remoting.Messaging;
    using System.Runtime.Remoting;
#elif NETSTANDARD1_5
    using System.Threading;
#endif
    using Contracts;
    using Microsoft.Extensions.Caching.Memory;

    public class MockedMemoryCache : IMockedMemoryCache
    {
#if NET451
        private const string DataKey = "__MemoryCache_Current__";
#elif NETSTANDARD1_5
        private static readonly AsyncLocal<IDictionary<object, ICacheEntry>> МemoryCacheCurrent = new AsyncLocal<IDictionary<object, ICacheEntry>>();
#endif
        private readonly IDictionary<object, ICacheEntry> cache;

        public MockedMemoryCache()
        {
            this.cache = this.GetCurrentCache();
        }

        public int Count => this.cache.Count;

        public void Dispose()
        {
            this.cache.Clear();
        }

        public void Remove(object key)
        {
            if (this.cache.ContainsKey(key))
            {
                this.cache.Remove(key);
            }
        }

        public ICacheEntry CreateEntry(object key)
        {
            var value = new MockedCacheEntry(key);
            this.cache[key] = value;
            return value;
        }

        public bool TryGetValue(object key, out object value)
        {
            ICacheEntry cacheEntry;
            if (this.TryGetCacheEntry(key, out cacheEntry))
            {
                value = cacheEntry.Value;
                return true;
            }

            value = null;
            return false;
        }

        public bool TryGetCacheEntry(object key, out ICacheEntry value)
        {
            if (this.cache.ContainsKey(key))
            {
                value = this.cache[key];
                return true;
            }
            else
            {
                value = null;
                return false;
            }
        }

        public IDictionary<object, object> GetCacheAsDictionary()
        {
            return this.cache.ToDictionary(c => c.Key, c => c.Value.Value);
        }

        private IDictionary<object, ICacheEntry> GetCurrentCache()
        {
#if NET451
            var handle = CallContext.GetData(DataKey) as ObjectHandle;
            var result = handle?.Unwrap() as IDictionary<object, ICacheEntry>;
            if (result == null)
            {
                result = new Dictionary<object, ICacheEntry>();
                CallContext.SetData(DataKey, new ObjectHandle(result));
            }

            return result;
#elif NETSTANDARD1_5
            var result = МemoryCacheCurrent.Value;
            if (result == null)
            {
                result = new Dictionary<object, ICacheEntry>();
                МemoryCacheCurrent.Value = result;
            }

            return result;
#endif
        }
    }
}

The following test runs successfully on DNX. Assume that TestServiceProvider always returns the same instanve of MockedMemoryCache and TestHelper just call Dispose, which clears the dictionary.

        [Fact]
        public void MockedMemoryCacheShouldBeDifferentForEveryCallAsynchronously()
        {
            Task
                .Run(async () =>
                {
                    TestHelper.ClearMemoryCache();

                    string firstValue = null;
                    string secondValue = null;
                    string thirdValue = null;
                    string fourthValue = null;
                    string fifthValue = null;

                    var tasks = new List<Task>
                    {
                        Task.Run(() =>
                        {
                            var memoryCache = TestServiceProvider.GetService<IMemoryCache>();
                            memoryCache.Set("test", "first");
                            firstValue = TestServiceProvider.GetService<IMemoryCache>().Get<string>("test");
                            TestHelper.ClearMemoryCache();
                        }),
                        Task.Run(() =>
                        {
                            var memoryCache = TestServiceProvider.GetService<IMemoryCache>();
                            memoryCache.Set("test", "second");
                            secondValue = TestServiceProvider.GetService<IMemoryCache>().Get<string>("test");
                            TestHelper.ClearMemoryCache();
                        }),
                        Task.Run(() =>
                        {
                            var memoryCache = TestServiceProvider.GetService<IMemoryCache>();
                            memoryCache.Set("test", "third");
                            thirdValue = TestServiceProvider.GetService<IMemoryCache>().Get<string>("test");
                            TestHelper.ClearMemoryCache();
                        }),
                        Task.Run(() =>
                        {
                            var memoryCache = TestServiceProvider.GetService<IMemoryCache>();
                            memoryCache.Set("test", "fourth");
                            fourthValue = TestServiceProvider.GetService<IMemoryCache>().Get<string>("test");
                            TestHelper.ClearMemoryCache();
                        }),
                        Task.Run(() =>
                        {
                            var memoryCache = TestServiceProvider.GetService<IMemoryCache>();
                            memoryCache.Set("test", "fifth");
                            fifthValue = TestServiceProvider.GetService<IMemoryCache>().Get<string>("test");
                            TestHelper.ClearMemoryCache();
                        })
                    };

                    await Task.WhenAll(tasks);

                    Assert.Equal("first", firstValue);
                    Assert.Equal("second", secondValue);
                    Assert.Equal("third", thirdValue);
                    Assert.Equal("fourth", fourthValue);
                    Assert.Equal("fifth", fifthValue);
                })
                .GetAwaiter()
                .GetResult();
        }

Expected behavior

The test to pass like it did on DNX.

Actual behavior

After moving to CLI the test started failing with unexpected values like first equals third and so on. It seems that the internal dictionary is shared between the tasks and it should not be.

Maybe I am missing something? Is this expected behavior? If you need more minimalistic example, I can provide one.

NOTE: After changing AsyncLocal to ThreadLocal the test passed right away.

joshfree commented 8 years ago

cc: @sergiy-k

karelz commented 7 years ago

Did you get chance to debug it and find out what is wrong?

hikalkan commented 7 years ago

I have a similar problem. When I switch to ThreadLocal it works, but AsyncLocal does not.

hikalkan commented 7 years ago

A very simple code to re-produce it:

    public class AsyncLocal_Tests
    {
        private static readonly AsyncLocal<string> _asyncLocal = new AsyncLocal<string>();

        [Fact]
        public async Task Test1()
        {
            await AsyncTestCode("42");
            _asyncLocal.Value.ShouldBe("42"); //TEST FAILS IN THIS POINT... IT'S NULL!
        }

        private static async Task AsyncTestCode(string value)
        {
            using (var ms = new MemoryStream())
            {
                await ms.WriteAsync(new[] { (byte)1 }, 0, 1);

                _asyncLocal.Value = value;
                _asyncLocal.Value.ShouldBe(value);

                await ms.WriteAsync(new[] { (byte)2 }, 0, 1);
            }
        }
    }
stephentoub commented 7 years ago

This is the expected behavior. Any changes made to _asyncLocal.Value inside of an async method do not propagate back out to the synchronous caller of the method. There is explicit code in the infrastructure for async methods to prevent that from happening.

hikalkan commented 7 years ago

Thanks @stephentoub for the explanation. But then how I can achive the scenario above? I was using CallContext.LogicalSetData before, but it's not available in .netcore/.netstandard.

karelz commented 7 years ago

Did you consider simply returning value from the async method? (or use out/ref parameters if you have more values)

hikalkan commented 7 years ago

This is a very very simplified example to demonstrate the problem. My actual application/framework is much more complicated. What I want to have a ambient value shared in current thread/async-flow. In a web application, we can store such a value in HttpContext.Items. But if we have a background job or console app, this is also not possible. So, what I was looking for a good replacement for CallContext like in .net framework.

kouvel commented 7 years ago

You could do something like this:

    public static class AsyncLocal_Tests
    {
        private static readonly AsyncLocal<MyAsyncFlowState> _asyncLocal = new AsyncLocal<MyAsyncFlowState>();

        private class MyAsyncFlowState
        {
            public string Str { get; set; }
        }

        [Fact]
        public static async Task Test1()
        {
            _asyncLocal.Value = new MyAsyncFlowState();
            await AsyncTestCode("42");
            Console.WriteLine(_asyncLocal.Value.Str ?? "(null)");
        }

        private static async Task AsyncTestCode(string value)
        {
            using (var ms = new MemoryStream())
            {
                await ms.WriteAsync(new[] { (byte)1 }, 0, 1);

                _asyncLocal.Value.Str = value;
                Console.WriteLine(_asyncLocal.Value.Str ?? "(null)");

                await ms.WriteAsync(new[] { (byte)2 }, 0, 1);
            }
        }
    }
hikalkan commented 7 years ago

But this is not thread safe. Many thread can access to _asyncLocal.Value.Str concurrently and overwrite each other's values. Am I wrong?

kouvel commented 7 years ago

_asyncLocal.Value would be unique per ExecutionContext, but in the async flow there could be multiple threads trying to access/change it. In your original code, changing _asyncLocal.Value would change the current execution context, leaving the original execution context unchanged, which is why you don't see the updated value in the completion. Suppose it were instead to change the same execution context, you would then have the same thread safety issue. If you need thread safety, you may need to add it into MyAsyncFlowState.

hikalkan commented 7 years ago

I will think on that, thanks a lot.

hikalkan commented 7 years ago

I think this there is still a problem. We may need to change ambient value from inner method and expect to effect the value in the containing method. .net framework should provide a way of that.

stephentoub commented 7 years ago

We may need to change ambient value from inner method and expect to effect the value in the containing method. .net framework should provide a way of that.

It's explicitly by design that you can't do that from inside of an async method, so it's not a bug. If you want to do that, you can't use an async method (it can still be Task-returning, just not using the async/await keywords).

hikalkan commented 7 years ago

OK, understand that this is by design. But, is there any other way of doing that (beside AsyncLocal)?

stephentoub commented 7 years ago

Every mechanism for flowing state across asynchronous points (e.g. AsyncLocal, CallContext, etc.) does so via ExecutionContext, and it's ExecutionContext that is prevented from flowing out of an async/await method. So any changes done via any such mechanism inside of an async/await method will not flow out. If you want changes to AsyncLocal, CallContext, etc. to flow out of an asynchronous method to its synchronous caller, that asynchronous method can't itself be marked as async... it could still be Task-returning, and it could make a change to something and then delegate to an async/await method, and the changes would propagate out to the synchronous caller fine.

hikalkan commented 7 years ago

Thank you for detailer explanation and for your valuable time. I tested and see that it's same for CallContext too. So, there is no way of what I want and should make it working with given rule.

stephentoub commented 7 years ago

@hikalkan, happy to help, thanks.