Switched to AsyncKeyedLock

[MarkCiliaVincenti]

This implementation will give you lower memory allocations.

[artmasa]

Interesting. Let me take a look at this! Thanks!

[artmasa]

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.

[MarkCiliaVincenti]

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.

[MarkCiliaVincenti]

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.

[artmasa]

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.

[MarkCiliaVincenti]

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.

[MarkCiliaVincenti]

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.

[artmasa]

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.

[MarkCiliaVincenti]

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.

[MarkCiliaVincenti]

Shall we revisit this @artmasa ?