Skip to content

Switched to AsyncKeyedLock #49

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Switched to AsyncKeyedLock #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/abstractions/DarkLoop.Azure.Functions.Authorization.Abstractions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

<ItemGroup>
<FrameworkReference Include="Microsoft.AspNetCore.App" />
<PackageReference Include="AsyncKeyedLock" Version="6.4.2" />
<PackageReference Include="Microsoft.AspNetCore.Authorization" Version="3.0.3" />
<PackageReference Include="Microsoft.AspNetCore.Metadata" Version="3.0.3" />
</ItemGroup>
Expand Down
4 changes: 3 additions & 1 deletion src/abstractions/FunctionsAuthorizationCoreServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Copyright (c) DarkLoop. All rights reserved.
// </copyright>

using AsyncKeyedLock;
using DarkLoop.Azure.Functions.Authorization.Cache;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
Expand All @@ -20,7 +21,8 @@ public static IServiceCollection AddFunctionsAuthorizationCore(this IServiceColl

return services
.AddSingleton<IFunctionsAuthorizationResultHandler, FunctionsAuthorizationResultHandler>()
.AddSingleton(typeof(IFunctionsAuthorizationFilterCache<>), typeof(FunctionsAuthorizationFilterCache<>));
.AddSingleton(typeof(IFunctionsAuthorizationFilterCache<>), typeof(FunctionsAuthorizationFilterCache<>))
.AddSingleton(new AsyncKeyedLocker<string>(o => { o.PoolSize = 20; o.PoolInitialFill = 1; }));
Copy link
Contributor

@artmasa artmasa May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to named lockers? If someone using this package also references your package for their own purposes and add the locker to the services, it would be replacing this configuration.
KeyedMonitor is internal and there won't be clashes.
Can you look into instrumenting the locker where you have a single constructor and initializing it using the options pattern.

public class AsyncKeyedLocker<TKey>
{
  // This should be the only public constructor 
  public AsyncKeyedLocker(AsyncKeyedLockOptions options)
  {
      // init code
  }

  ...
}

and intrument it like

// this would require a factory type to get the named singleton
services.AddKeyedLocker<TKey>("<locker-name>", options => { ... });

// or

services.AddKeyedLocker<TKey, TService>(options => { ... });

the second one allows for automatic injection when used as constructor parameters for TService and services provides a unique instance for this service, so it becomes a singleton for the service instead of a singleton for the application.

The way it is right now does not ensure that external code will not change the service as this package requires it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand you correctly. You're creating an instance of AsyncKeyedLocker and someone in a different assembly might also do the same, but every instance of AsyncKeyedLocker will have its own dictionary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine we were just talking about a normal Dictionary here which you'd inject as a singleton. Unless you intentionally expose it no one will be able to modify your dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally within this package we call AddSingleton(new AsyncKeyedLocker<string>(options => ...)). This gets invoked on AddFunctionsAuthentication(... )

The problem is when a consumer of this package has

services
   .AddFunctionsAuthentication(); //Internally this adds the singleton descriptor for the locker

// a few lines below
services
   .AddSingleton<AsyncKeyedLocker<string>>(); // This one just replaced the one configured for functions authentication

It does not ensure the locker behavior for what this package needs will be kept intact if a user uses DarkLoop package and your package for any other purpose.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it shouldn't. It should be easy to test. I mean technically you could wrap the locker in an internal KeyedLock class but that's not going to change anything except for slightly reducing performance.

Copy link
Author

@MarkCiliaVincenti MarkCiliaVincenti May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the second AddSingleton will create a new instance not reuse an instance created by a different library. You can debug AsyncKeyedLocker so with that code you showed me you can see that it should go into the AsyncKeyedLock constructor twice, once for your library and once for wherever the library is used. That is you don't even need to set up locks or anything in order to test this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how ServiceCollection works. The last registered descriptor for a specific Type will be the one resolving that type. It does not depend on your code. This is why I'm asking if your component can add the functionality to ensure that.
At the moment, KeyedMonitor ensures the desired behavior for the component.
Hopefully you are able to add this functionality at some point to onboard this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a wrapper class for it if you're worried? What you're asking for is extremely unoptimal and you can already do it yourself by creating a Dictionary<string, AsyncKeyedLocker> but I'd advise to avoid that. Sorry for my tone, very very long day at work.

}
}
}
12 changes: 5 additions & 7 deletions src/abstractions/FunctionsAuthorizationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using AsyncKeyedLock;
using DarkLoop.Azure.Functions.Authorization.Cache;
using DarkLoop.Azure.Functions.Authorization.Internal;
using DarkLoop.Azure.Functions.Authorization.Properties;
Expand All @@ -26,6 +27,7 @@ internal class FunctionsAuthorizationProvider : IFunctionsAuthorizationProvider
private readonly IFunctionsAuthorizationFilterCache<int> _filterCache;
private readonly FunctionAuthorizationMetadataCollection _metadataStore;
private readonly IOptionsMonitor<FunctionsAuthorizationOptions> _options;
private readonly AsyncKeyedLocker<string> _asyncKeyedLocker;
private readonly ILogger _logger;

/// <summary>
Expand All @@ -41,6 +43,7 @@ public FunctionsAuthorizationProvider(
IFunctionsAuthorizationFilterCache<int> cache,
IOptions<FunctionsAuthorizationOptions> options,
IOptionsMonitor<FunctionsAuthorizationOptions> configOptions,
AsyncKeyedLocker<string> asyncKeyedLocker,
ILogger<IFunctionsAuthorizationProvider> logger)
{
Check.NotNull(schemeProvider, nameof(schemeProvider));
Expand All @@ -53,6 +56,7 @@ public FunctionsAuthorizationProvider(
_filterCache = cache;
_metadataStore = options.Value.AuthorizationMetadata;
_options = configOptions;
_asyncKeyedLocker = asyncKeyedLocker;
_logger = logger;
}

Expand All @@ -71,9 +75,7 @@ public async Task<FunctionAuthorizationFilter> GetAuthorizationAsync(string func

var asyncKey = $"fap:{functionName}";

await KeyedMonitor.EnterAsync(asyncKey, unblockOnFirstExit: true);

try
using (await _asyncKeyedLocker.LockAsync(asyncKey).ConfigureAwait(false))
{
if (_filterCache.TryGetFilter(key, out filter))
{
Expand Down Expand Up @@ -110,10 +112,6 @@ public async Task<FunctionAuthorizationFilter> GetAuthorizationAsync(string func

return filter;
}
finally
{
KeyedMonitor.Exit(asyncKey);
}
}

private async Task<AuthorizationPolicy?> GetPolicy(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authData)
Expand Down
117 changes: 0 additions & 117 deletions src/abstractions/Internal/KeyedMonitor.cs
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions test/Abstractions.Tests/FunctionsAuthorizationProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// </copyright>

using Abstractions.Tests.Fakes;
using AsyncKeyedLock;
using Common.Tests;
using DarkLoop.Azure.Functions.Authorization;
using DarkLoop.Azure.Functions.Authorization.Cache;
Expand Down Expand Up @@ -63,7 +64,7 @@ public async Task AuthorizationProviderShouldReturnFilterWithNoPolicyWhenMetadat

// Arrange
var provider = new FunctionsAuthorizationProvider(
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, _logger!);
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, new AsyncKeyedLocker<string>(), _logger!);

_onLog = (level, eventId, state, exception, formatter) =>
{
Expand Down Expand Up @@ -91,7 +92,7 @@ public async Task AuthorizationProviderShouldReturnFilterWithNoPolicyWhenMetadat

// Arrange
var provider = new FunctionsAuthorizationProvider(
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, _logger!);
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, new AsyncKeyedLocker<string>(), _logger!);

_onLog = (level, eventId, state, exception, formatter) =>
{
Expand Down Expand Up @@ -123,7 +124,7 @@ public async Task AuthorizationProviderShouldReturnFilterWithPolicyWhenMetadataP

// Arrange
var provider = new FunctionsAuthorizationProvider(
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, _logger!);
_schemeProvider!, new FunctionsAuthorizationFilterCache<int>(), _options!, _configOptions!, new AsyncKeyedLocker<string>(), _logger!);

_onLog = (level, eventId, state, exception, formatter) =>
{
Expand Down
21 changes: 8 additions & 13 deletions test/Abstractions.Tests/Internal/KeyedMonitorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,7 @@
// Copyright (c) DarkLoop. All rights reserved.
// </copyright>

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using DarkLoop.Azure.Functions.Authorization.Internal;
using AsyncKeyedLock;

namespace Abstractions.Tests.Internal
{
Expand All @@ -19,6 +14,7 @@ public class KeyedMonitorTests
private List<string>? _unmonitored;
private List<string>? _monitored;
private List<string>? _winner;
private AsyncKeyedLocker<string>? _asyncKeyedLocker;

[TestInitialize]
public void Initialize()
Expand All @@ -28,6 +24,11 @@ public void Initialize()
_unmonitored = new List<string>();
_monitored = new List<string>();
_winner = new List<string>();
_asyncKeyedLocker = new(o =>
{
o.PoolSize = 20;
o.PoolInitialFill = 1;
});
}

[TestMethod("KeyedMonitor: should allow for other threads to unblock after first exit")]
Expand Down Expand Up @@ -75,9 +76,7 @@ private async Task MonitoredLogicAsync(string name, int millisecondsToBlock)
return;
}

await KeyedMonitor.EnterAsync("x", unblockOnFirstExit: true);

try
using (await _asyncKeyedLocker.LockAsync("x"))
{
await Task.Delay(millisecondsToBlock);
_monitored!.Add(name);
Expand All @@ -90,10 +89,6 @@ private async Task MonitoredLogicAsync(string name, int millisecondsToBlock)
_flag = true;
_winner!.Add(name);
}
finally
{
KeyedMonitor.Exit("x");
}
}
}
}