From c61c342287013d3a3360adfa7f612919f3b95220 Mon Sep 17 00:00:00 2001 From: Martin Obratil Date: Mon, 15 Jan 2024 21:48:05 +0100 Subject: [PATCH 1/3] Updated README.md files with notes mentioning thread-safety considerations --- .../Microsoft.AspNetCore.AsyncState/README.md | 3 ++ .../AsyncState.cs | 31 ++++++++++--- .../Features.cs | 45 ------------------- .../FeaturesPooledPolicy.cs | 15 ++++--- .../Microsoft.Extensions.AsyncState/README.md | 4 +- .../AsyncStateTests.cs | 25 +++++++++++ .../FeaturesPooledPolicyTests.cs | 17 ++++--- 7 files changed, 73 insertions(+), 67 deletions(-) delete mode 100644 src/Libraries/Microsoft.Extensions.AsyncState/Features.cs diff --git a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md index d5bc4f0a353..d53fafcbfd5 100644 --- a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md +++ b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md @@ -4,6 +4,9 @@ This provides the ability to store and retrieve state objects that flow with the The lifetime of the shared data is controlled automatically and will be the same as of `HttpContext`. +> [!NOTE] +> Please note, the implementation `IAsyncContext` is not designed to be thread-safe. + ## Install the package From the command-line: diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/AsyncState.cs b/src/Libraries/Microsoft.Extensions.AsyncState/AsyncState.cs index 9fc79930954..4d782409238 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/AsyncState.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/AsyncState.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Threading; using Microsoft.Extensions.ObjectPool; using Microsoft.Shared.Diagnostics; @@ -12,7 +13,7 @@ namespace Microsoft.Extensions.AsyncState; internal sealed class AsyncState : IAsyncState { private static readonly AsyncLocal _asyncContextCurrent = new(); - private static readonly ObjectPool _featuresPool = PoolFactory.CreatePool(new FeaturesPooledPolicy()); + private static readonly ObjectPool> _featuresPool = PoolFactory.CreatePool(new FeaturesPooledPolicy()); private int _contextCount; public void Initialize() @@ -21,12 +22,12 @@ public void Initialize() // Use an object indirection to hold the AsyncContext in the AsyncLocal, // so it can be cleared in all ExecutionContexts when its cleared. - var asyncStateHolder = new AsyncStateHolder + var features = new AsyncStateHolder { Features = _featuresPool.Get() }; - _asyncContextCurrent.Value = asyncStateHolder; + _asyncContextCurrent.Value = features; } public void Reset() @@ -59,7 +60,9 @@ public bool TryGet(AsyncStateToken token, out object? value) return false; } - value = _asyncContextCurrent.Value.Features.Get(token.Index); + EnsureCount(_asyncContextCurrent.Value.Features, token.Index + 1); + + value = _asyncContextCurrent.Value.Features[token.Index]; return true; } @@ -83,14 +86,28 @@ public void Set(AsyncStateToken token, object? value) Throw.InvalidOperationException("Context is not initialized"); } - _asyncContextCurrent.Value.Features.Set(token.Index, value); + EnsureCount(_asyncContextCurrent.Value.Features, token.Index + 1); + + _asyncContextCurrent.Value.Features[token.Index] = value; + } + + internal static void EnsureCount(List features, int count) + { +#if NET6_0_OR_GREATER + features.EnsureCapacity(count); +#endif + var difference = count - features.Count; + + for (int i = 0; i < difference; i++) + { + features.Add(null); + } } internal int ContextCount => Volatile.Read(ref _contextCount); private sealed class AsyncStateHolder { - public Features? Features { get; set; } + public List? Features { get; set; } } - } diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/Features.cs b/src/Libraries/Microsoft.Extensions.AsyncState/Features.cs deleted file mode 100644 index 940efa69c26..00000000000 --- a/src/Libraries/Microsoft.Extensions.AsyncState/Features.cs +++ /dev/null @@ -1,45 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Collections.Generic; - -namespace Microsoft.Extensions.AsyncState; - -internal sealed class Features -{ - private readonly List _items = []; - - public object? Get(int index) - { - return _items.Count <= index ? null : _items[index]; - } - - public void Set(int index, object? value) - { - if (_items.Count <= index) - { - lock (_items) - { - var count = index + 1; - -#if NET6_0_OR_GREATER - _items.EnsureCapacity(count); -#endif - - var difference = count - _items.Count; - - for (int i = 0; i < difference; i++) - { - _items.Add(null); - } - } - } - - _items[index] = value; - } - - public void Clear() - { - _items.Clear(); - } -} diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/FeaturesPooledPolicy.cs b/src/Libraries/Microsoft.Extensions.AsyncState/FeaturesPooledPolicy.cs index 6c6f442c817..3b045465595 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/FeaturesPooledPolicy.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/FeaturesPooledPolicy.cs @@ -1,22 +1,27 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using Microsoft.Extensions.ObjectPool; namespace Microsoft.Extensions.AsyncState; -internal sealed class FeaturesPooledPolicy : IPooledObjectPolicy +internal sealed class FeaturesPooledPolicy : IPooledObjectPolicy> { /// - public Features Create() + public List Create() { - return new Features(); + return []; } /// - public bool Return(Features obj) + public bool Return(List obj) { - obj.Clear(); + for (int i = 0; i < obj.Count; i++) + { + obj[i] = null; + } + return true; } } diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/README.md b/src/Libraries/Microsoft.Extensions.AsyncState/README.md index a58555d0389..57b93fdc4e1 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/README.md +++ b/src/Libraries/Microsoft.Extensions.AsyncState/README.md @@ -1,12 +1,14 @@ # Microsoft.Extensions.AsyncState This provides the ability to store and retrieve objects that flow with the current asynchronous context. - It has a few advantages over using the [`AsyncLocal`](https://learn.microsoft.com/dotnet/api/system.threading.asynclocal-1) class directly: - By abstracting the way the ambient data is stored we can use more optimized implementations, for instance when using ASP.NET Core, without exposing these components. - Improves the performance by minimizing the number of `AsyncLocal` instances required when multiple objects are shared. - Provides a way to manage the lifetime of the ambient data objects. +> [!NOTE] +> Please note, the implementations of `IAsyncState` and `IAsyncContext` are not designed to be thread-safe. + ## Install the package From the command-line: diff --git a/test/Libraries/Microsoft.Extensions.AsyncState.Tests/AsyncStateTests.cs b/test/Libraries/Microsoft.Extensions.AsyncState.Tests/AsyncStateTests.cs index e541ee04865..f7a7b050eb1 100644 --- a/test/Libraries/Microsoft.Extensions.AsyncState.Tests/AsyncStateTests.cs +++ b/test/Libraries/Microsoft.Extensions.AsyncState.Tests/AsyncStateTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; @@ -204,4 +205,28 @@ public void RegisterContextCorrectly() Assert.Equal(3, asyncState.ContextCount); } + + [Fact] + public void EnsureCount_IncreasesCountCorrectly() + { + var l = new List(); + AsyncState.EnsureCount(l, 5); + Assert.Equal(5, l.Count); + } + + [Fact] + public void EnsureCount_WhenCountLessThanExpected() + { + var l = new List(new object?[5]); + AsyncState.EnsureCount(l, 2); + Assert.Equal(5, l.Count); + } + + [Fact] + public void EnsureCount_WhenCountEqualWithExpected() + { + var l = new List(new object?[5]); + AsyncState.EnsureCount(l, 5); + Assert.Equal(5, l.Count); + } } diff --git a/test/Libraries/Microsoft.Extensions.AsyncState.Tests/FeaturesPooledPolicyTests.cs b/test/Libraries/Microsoft.Extensions.AsyncState.Tests/FeaturesPooledPolicyTests.cs index 6e360789cd9..909389acb95 100644 --- a/test/Libraries/Microsoft.Extensions.AsyncState.Tests/FeaturesPooledPolicyTests.cs +++ b/test/Libraries/Microsoft.Extensions.AsyncState.Tests/FeaturesPooledPolicyTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using Xunit; namespace Microsoft.Extensions.AsyncState.Test; @@ -12,7 +13,7 @@ public void Return_ShouldBeTrue() { var policy = new FeaturesPooledPolicy(); - Assert.True(policy.Return(new Features())); + Assert.True(policy.Return([])); } [Fact] @@ -20,14 +21,12 @@ public void Return_ShouldNullList() { var policy = new FeaturesPooledPolicy(); - var features = policy.Create(); - features.Set(0, string.Empty); - features.Set(1, Array.Empty()); - features.Set(2, new object()); + var list = policy.Create(); + list.Add(string.Empty); + list.Add(Array.Empty()); + list.Add(new object()); - Assert.True(policy.Return(features)); - Assert.Null(features.Get(0)); - Assert.Null(features.Get(1)); - Assert.Null(features.Get(2)); + Assert.True(policy.Return(list)); + Assert.All(list, el => Assert.Null(el)); } } From 0800989c17a7114cc8017c00e6a997c79bbeeaa1 Mon Sep 17 00:00:00 2001 From: Martin Obratil Date: Mon, 15 Jan 2024 22:23:24 +0100 Subject: [PATCH 2/3] Extended XML doc to explicitly mention the implementations of IAsyncContext, IAsyncState and IAsyncLocalContext are not thread safe. --- .../AsyncStateHttpContextExtensions.cs | 1 + src/Libraries/Microsoft.AspNetCore.AsyncState/README.md | 2 +- .../Microsoft.Extensions.AsyncState/AsyncStateExtensions.cs | 1 + src/Libraries/Microsoft.Extensions.AsyncState/IAsyncContext.cs | 1 + .../Microsoft.Extensions.AsyncState/IAsyncLocalContext.cs | 1 + src/Libraries/Microsoft.Extensions.AsyncState/IAsyncState.cs | 1 + 6 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Libraries/Microsoft.AspNetCore.AsyncState/AsyncStateHttpContextExtensions.cs b/src/Libraries/Microsoft.AspNetCore.AsyncState/AsyncStateHttpContextExtensions.cs index 6c289c96f77..d9141dbf83e 100644 --- a/src/Libraries/Microsoft.AspNetCore.AsyncState/AsyncStateHttpContextExtensions.cs +++ b/src/Libraries/Microsoft.AspNetCore.AsyncState/AsyncStateHttpContextExtensions.cs @@ -16,6 +16,7 @@ public static class AsyncStateHttpContextExtensions /// /// Adds default implementations for , , and services, /// scoped to the lifetime of instances. + /// Please note that implementations of these interfaces are not thread safe. /// /// The to add the service to. /// The value of . diff --git a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md index d53fafcbfd5..ec93c727704 100644 --- a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md +++ b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md @@ -5,7 +5,7 @@ This provides the ability to store and retrieve state objects that flow with the The lifetime of the shared data is controlled automatically and will be the same as of `HttpContext`. > [!NOTE] -> Please note, the implementation `IAsyncContext` is not designed to be thread-safe. +> Please note, the implementation of `IAsyncContext` is not designed to be thread-safe. ## Install the package diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/AsyncStateExtensions.cs b/src/Libraries/Microsoft.Extensions.AsyncState/AsyncStateExtensions.cs index 9baabc82d3a..a25a8156b1a 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/AsyncStateExtensions.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/AsyncStateExtensions.cs @@ -15,6 +15,7 @@ public static class AsyncStateExtensions { /// /// Adds default implementations for , , and services. + /// Please note that implementations of these interfaces are not thread safe. /// /// The dependency injection container to add the implementations to. /// The value of . diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncContext.cs b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncContext.cs index f1c07ee94f8..aef151f3016 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncContext.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncContext.cs @@ -8,6 +8,7 @@ namespace Microsoft.Extensions.AsyncState; /// /// Provides access to the current async context. +/// Some implementations of this interface may not be thread safe. /// /// The type of the asynchronous state. [SuppressMessage("Naming", "CA1716:Identifiers should not match keywords", Justification = "Getter and setter throw exceptions.")] diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncLocalContext.cs b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncLocalContext.cs index 746909d1b8c..9873375a78b 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncLocalContext.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncLocalContext.cs @@ -7,6 +7,7 @@ namespace Microsoft.Extensions.AsyncState; /// /// Provides access to the current async context stored outside of the HTTP pipeline. +/// Some implementations of this interface may not be thread safe. /// /// The type of the asynchronous state. /// This type is intended for internal use. Use instead. diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncState.cs b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncState.cs index 960d6672967..ec15b72c2c9 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncState.cs +++ b/src/Libraries/Microsoft.Extensions.AsyncState/IAsyncState.cs @@ -9,6 +9,7 @@ namespace Microsoft.Extensions.AsyncState; /// /// Encapsulates all information within the asynchronous flow in an variable. +/// Some implementations of this interface may not be thread safe. /// [SuppressMessage("Naming", "CA1716:Identifiers should not match keywords", Justification = "Getter and setter throw exceptions.")] public interface IAsyncState From 6ed0e2c14ca3fd1a07be5c384e243627442fe09e Mon Sep 17 00:00:00 2001 From: Martin Obratil Date: Mon, 15 Jan 2024 22:53:23 +0100 Subject: [PATCH 3/3] Update thread-safety note in docs --- src/Libraries/Microsoft.AspNetCore.AsyncState/README.md | 2 +- src/Libraries/Microsoft.Extensions.AsyncState/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md index ec93c727704..5bce6aa3a99 100644 --- a/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md +++ b/src/Libraries/Microsoft.AspNetCore.AsyncState/README.md @@ -5,7 +5,7 @@ This provides the ability to store and retrieve state objects that flow with the The lifetime of the shared data is controlled automatically and will be the same as of `HttpContext`. > [!NOTE] -> Please note, the implementation of `IAsyncContext` is not designed to be thread-safe. +> Please note, the implementation of `IAsyncContext` provided by this library is not thread-safe. ## Install the package diff --git a/src/Libraries/Microsoft.Extensions.AsyncState/README.md b/src/Libraries/Microsoft.Extensions.AsyncState/README.md index 57b93fdc4e1..46a0c434d61 100644 --- a/src/Libraries/Microsoft.Extensions.AsyncState/README.md +++ b/src/Libraries/Microsoft.Extensions.AsyncState/README.md @@ -7,7 +7,7 @@ It has a few advantages over using the [`AsyncLocal`](https://learn.microsoft - Provides a way to manage the lifetime of the ambient data objects. > [!NOTE] -> Please note, the implementations of `IAsyncState` and `IAsyncContext` are not designed to be thread-safe. +> Please note, the implementations of `IAsyncState` and `IAsyncContext` are not thread-safe. ## Install the package