From c792389b41226c13009d6f8fa980ca521996f401 Mon Sep 17 00:00:00 2001 From: maliming Date: Mon, 17 May 2021 15:48:51 +0800 Subject: [PATCH 1/5] Use locks to prevent concurrency confusion. I will re-think this design. --- .../Toolbars/ToolbarManager.cs | 21 ++++++++++++------- .../Volo/Abp/Ui/Navigation/MenuManager.cs | 21 ++++++++++++------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs b/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs index ecd73ac5ab..d8a0e68842 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -8,6 +9,7 @@ using Volo.Abp.AspNetCore.Mvc.UI.Theming; using Volo.Abp.Authorization.Permissions; using Volo.Abp.DependencyInjection; using Volo.Abp.SimpleStateChecking; +using Volo.Abp.Threading; namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars { @@ -17,6 +19,7 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars protected AbpToolbarOptions Options { get; } protected IServiceProvider ServiceProvider { get; } protected ISimpleStateCheckerManager SimpleStateCheckerManager { get; } + protected SemaphoreSlim SyncSemaphore { get; } public ToolbarManager( IOptions options, @@ -28,6 +31,7 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars SimpleStateCheckerManager = simpleStateCheckerManager; ServiceProvider = serviceProvider; Options = options.Value; + SyncSemaphore = new SemaphoreSlim(1, 1); } public async Task GetAsync(string name) @@ -36,16 +40,19 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars using (var scope = ServiceProvider.CreateScope()) { - RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); + using (await SyncSemaphore.LockAsync()) + { + RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); - var context = new ToolbarConfigurationContext(ThemeManager.CurrentTheme, toolbar, scope.ServiceProvider); + var context = new ToolbarConfigurationContext(ThemeManager.CurrentTheme, toolbar, scope.ServiceProvider); - foreach (var contributor in Options.Contributors) - { - await contributor.ConfigureToolbarAsync(context); - } + foreach (var contributor in Options.Contributors) + { + await contributor.ConfigureToolbarAsync(context); + } - await CheckPermissionsAsync(scope.ServiceProvider, toolbar); + await CheckPermissionsAsync(scope.ServiceProvider, toolbar); + } } return toolbar; diff --git a/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs b/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs index d58c88b6e6..026a41999c 100644 --- a/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs +++ b/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs @@ -1,11 +1,13 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Options; using Volo.Abp.Authorization.Permissions; using Volo.Abp.DependencyInjection; using Volo.Abp.SimpleStateChecking; +using Volo.Abp.Threading; namespace Volo.Abp.UI.Navigation { @@ -14,6 +16,7 @@ namespace Volo.Abp.UI.Navigation protected AbpNavigationOptions Options { get; } protected IHybridServiceScopeFactory ServiceScopeFactory { get; } protected ISimpleStateCheckerManager SimpleStateCheckerManager { get; } + protected SemaphoreSlim SyncSemaphore { get; } public MenuManager( IOptions options, @@ -23,6 +26,7 @@ namespace Volo.Abp.UI.Navigation Options = options.Value; ServiceScopeFactory = serviceScopeFactory; SimpleStateCheckerManager = simpleStateCheckerManager; + SyncSemaphore = new SemaphoreSlim(1, 1); } public async Task GetAsync(string name) @@ -31,16 +35,19 @@ namespace Volo.Abp.UI.Navigation using (var scope = ServiceScopeFactory.CreateScope()) { - RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); + using (await SyncSemaphore.LockAsync()) + { + RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); - var context = new MenuConfigurationContext(menu, scope.ServiceProvider); + var context = new MenuConfigurationContext(menu, scope.ServiceProvider); - foreach (var contributor in Options.MenuContributors) - { - await contributor.ConfigureMenuAsync(context); - } + foreach (var contributor in Options.MenuContributors) + { + await contributor.ConfigureMenuAsync(context); + } - await CheckPermissionsAsync(scope.ServiceProvider, menu); + await CheckPermissionsAsync(scope.ServiceProvider, menu); + } } NormalizeMenu(menu); From 7b8d127615b4a71c07ba708c29e593015af8b4ed Mon Sep 17 00:00:00 2001 From: maliming Date: Mon, 17 May 2021 16:33:34 +0800 Subject: [PATCH 2/5] Use AsyncLocal to store static checker. --- .../Toolbars/ToolbarManager.cs | 8 +------- .../PermissionSimpleStateCheckerExtensions.cs | 4 ++-- .../RequirePermissionsSimpleBatchStateChecker.cs | 11 ++++++++--- .../Volo/Abp/Ui/Navigation/MenuManager.cs | 9 +-------- 4 files changed, 12 insertions(+), 20 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs b/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs index d8a0e68842..53679c5b22 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared/Toolbars/ToolbarManager.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -9,7 +8,6 @@ using Volo.Abp.AspNetCore.Mvc.UI.Theming; using Volo.Abp.Authorization.Permissions; using Volo.Abp.DependencyInjection; using Volo.Abp.SimpleStateChecking; -using Volo.Abp.Threading; namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars { @@ -19,7 +17,6 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars protected AbpToolbarOptions Options { get; } protected IServiceProvider ServiceProvider { get; } protected ISimpleStateCheckerManager SimpleStateCheckerManager { get; } - protected SemaphoreSlim SyncSemaphore { get; } public ToolbarManager( IOptions options, @@ -31,7 +28,6 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars SimpleStateCheckerManager = simpleStateCheckerManager; ServiceProvider = serviceProvider; Options = options.Value; - SyncSemaphore = new SemaphoreSlim(1, 1); } public async Task GetAsync(string name) @@ -40,10 +36,8 @@ namespace Volo.Abp.AspNetCore.Mvc.UI.Theme.Shared.Toolbars using (var scope = ServiceProvider.CreateScope()) { - using (await SyncSemaphore.LockAsync()) + using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) { - RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); - var context = new ToolbarConfigurationContext(ThemeManager.CurrentTheme, toolbar, scope.ServiceProvider); foreach (var contributor in Options.Contributors) diff --git a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/PermissionSimpleStateCheckerExtensions.cs b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/PermissionSimpleStateCheckerExtensions.cs index 5fc22cb93c..c8c4d315fd 100644 --- a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/PermissionSimpleStateCheckerExtensions.cs +++ b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/PermissionSimpleStateCheckerExtensions.cs @@ -43,8 +43,8 @@ namespace Volo.Abp.Authorization.Permissions if (batchCheck) { - RequirePermissionsSimpleBatchStateChecker.Instance.AddCheckModels(new RequirePermissionsSimpleBatchStateCheckerModel(state, permissions, requiresAll)); - state.StateCheckers.Add(RequirePermissionsSimpleBatchStateChecker.Instance); + RequirePermissionsSimpleBatchStateChecker.Current.AddCheckModels(new RequirePermissionsSimpleBatchStateCheckerModel(state, permissions, requiresAll)); + state.StateCheckers.Add(RequirePermissionsSimpleBatchStateChecker.Current); } else { diff --git a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs index ebd91bf14f..c8fc37d450 100644 --- a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs +++ b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs @@ -1,5 +1,7 @@ +using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Volo.Abp.SimpleStateChecking; @@ -9,7 +11,8 @@ namespace Volo.Abp.Authorization.Permissions public class RequirePermissionsSimpleBatchStateChecker : SimpleBatchStateCheckerBase where TState : IHasSimpleStateCheckers { - public static readonly RequirePermissionsSimpleBatchStateChecker Instance = new RequirePermissionsSimpleBatchStateChecker(); + public static RequirePermissionsSimpleBatchStateChecker Current => _current.Value; + private static readonly AsyncLocal> _current = new AsyncLocal>(); private readonly List> _models; @@ -26,9 +29,11 @@ namespace Volo.Abp.Authorization.Permissions return this; } - public void ClearCheckModels() + public static IDisposable Use(RequirePermissionsSimpleBatchStateChecker checker) { - _models.Clear(); + var previousValue = Current; + _current.Value = checker; + return new DisposeAction(() => _current.Value = previousValue); } public override async Task> IsEnabledAsync(SimpleBatchStateCheckerContext context) diff --git a/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs b/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs index 026a41999c..0b52aff7b8 100644 --- a/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs +++ b/framework/src/Volo.Abp.UI.Navigation/Volo/Abp/Ui/Navigation/MenuManager.cs @@ -1,13 +1,11 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Options; using Volo.Abp.Authorization.Permissions; using Volo.Abp.DependencyInjection; using Volo.Abp.SimpleStateChecking; -using Volo.Abp.Threading; namespace Volo.Abp.UI.Navigation { @@ -16,8 +14,6 @@ namespace Volo.Abp.UI.Navigation protected AbpNavigationOptions Options { get; } protected IHybridServiceScopeFactory ServiceScopeFactory { get; } protected ISimpleStateCheckerManager SimpleStateCheckerManager { get; } - protected SemaphoreSlim SyncSemaphore { get; } - public MenuManager( IOptions options, IHybridServiceScopeFactory serviceScopeFactory, @@ -26,7 +22,6 @@ namespace Volo.Abp.UI.Navigation Options = options.Value; ServiceScopeFactory = serviceScopeFactory; SimpleStateCheckerManager = simpleStateCheckerManager; - SyncSemaphore = new SemaphoreSlim(1, 1); } public async Task GetAsync(string name) @@ -35,10 +30,8 @@ namespace Volo.Abp.UI.Navigation using (var scope = ServiceScopeFactory.CreateScope()) { - using (await SyncSemaphore.LockAsync()) + using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) { - RequirePermissionsSimpleBatchStateChecker.Instance.ClearCheckModels(); - var context = new MenuConfigurationContext(menu, scope.ServiceProvider); foreach (var contributor in Options.MenuContributors) From afc4d734636a6a7ba54bd9b707b02d49a7719a41 Mon Sep 17 00:00:00 2001 From: maliming Date: Mon, 17 May 2021 17:08:38 +0800 Subject: [PATCH 3/5] Init AsyncLocal. --- .../RequirePermissionsSimpleBatchStateChecker.cs | 5 +++++ ...quirePermissionsSimpleBatchStateChecker_Tests.cs | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs index c8fc37d450..e03b2b87e3 100644 --- a/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs +++ b/framework/src/Volo.Abp.Authorization/Volo/Abp/Authorization/Permissions/RequirePermissionsSimpleBatchStateChecker.cs @@ -16,6 +16,11 @@ namespace Volo.Abp.Authorization.Permissions private readonly List> _models; + static RequirePermissionsSimpleBatchStateChecker() + { + _current.Value = new RequirePermissionsSimpleBatchStateChecker(); + } + public RequirePermissionsSimpleBatchStateChecker() { _models = new List>(); diff --git a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs index 6d75a80f1c..acfc0f9444 100644 --- a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs +++ b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs @@ -16,6 +16,19 @@ namespace Volo.Abp.Authorization _simpleStateCheckerManager = GetRequiredService>(); } + [Fact] + public void Switch_Current_Checker_Test() + { + var checker = RequirePermissionsSimpleBatchStateChecker.Current; + checker.ShouldNotBeNull(); + + using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) + { + RequirePermissionsSimpleBatchStateChecker.Current.ShouldNotBeNull(); + RequirePermissionsSimpleBatchStateChecker.Current.ShouldNotBe(checker); + } + } + [Fact] public async Task RequirePermissionsSimpleBatchStateChecker_Test() { From cc6c8dfb5b87aa124855c131460fd5233a3bc7ef Mon Sep 17 00:00:00 2001 From: maliming Date: Mon, 17 May 2021 18:34:42 +0800 Subject: [PATCH 4/5] Update RequirePermissionsSimpleBatchStateChecker_Tests.cs --- .../RequirePermissionsSimpleBatchStateChecker_Tests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs index acfc0f9444..29d542762c 100644 --- a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs +++ b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs @@ -20,7 +20,6 @@ namespace Volo.Abp.Authorization public void Switch_Current_Checker_Test() { var checker = RequirePermissionsSimpleBatchStateChecker.Current; - checker.ShouldNotBeNull(); using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) { From 5342a574ceb99d78f0854810ca8f8c9e49dd44f2 Mon Sep 17 00:00:00 2001 From: maliming Date: Tue, 18 May 2021 09:12:35 +0800 Subject: [PATCH 5/5] Update Switch_Current_Checker_Test, --- ...ermissionsSimpleBatchStateChecker_Tests.cs | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs index 29d542762c..a3b7a96eea 100644 --- a/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs +++ b/framework/test/Volo.Abp.Authorization.Tests/Volo/Abp/Authorization/RequirePermissionsSimpleBatchStateChecker_Tests.cs @@ -19,13 +19,20 @@ namespace Volo.Abp.Authorization [Fact] public void Switch_Current_Checker_Test() { - var checker = RequirePermissionsSimpleBatchStateChecker.Current; + var checker = RequirePermissionsSimpleBatchStateChecker.Current; + checker.ShouldNotBeNull(); - using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) + RequirePermissionsSimpleBatchStateChecker checker2 = null; + + using (RequirePermissionsSimpleBatchStateChecker.Use(new RequirePermissionsSimpleBatchStateChecker())) { - RequirePermissionsSimpleBatchStateChecker.Current.ShouldNotBeNull(); - RequirePermissionsSimpleBatchStateChecker.Current.ShouldNotBe(checker); + checker2 = RequirePermissionsSimpleBatchStateChecker.Current; + checker2.ShouldNotBeNull(); + checker2.ShouldNotBe(checker); } + + checker2.ShouldNotBeNull(); + checker2.ShouldNotBe(checker); } [Fact] @@ -58,5 +65,15 @@ namespace Volo.Abp.Authorization StateCheckers = new List>(); } } + + class MyStateEntity2 : IHasSimpleStateCheckers + { + public List> StateCheckers { get; } + + public MyStateEntity2() + { + StateCheckers = new List>(); + } + } } }