1

When I instantiate the SemaphoreSlim class I'd like to use a configuration key for the initialCount argument so as if the value needs to change we do not need to do a complete rebuild.

My current implementation is effectively:

public class Handler
{
    private static SemaphoreSlim pool;
    private static readonly object lockObject = new();
    private static bool isInitialised;

    public Handler(IConfiguration configuration)
    {
        if(isInitialised) return;
        int poolSize = configuration.GetValue("PoolSize", 3);
        lock(lockObject)
        {
            pool ??= new SemaphoreSlim(poolSize);
            isInitialised = true;
        }
    }
}

I feel a little uncomfortable with this approach and I wouldn't say I am hugely confident that it is the right solution.

Is there a better way to do this?

Robert Brend
  • 23
  • 4
  • 1
    Is the [double-checking-locking](https://en.wikipedia.org/wiki/Double-checked_locking) a problem or what? The best way to improve working code is [codereview](https://codereview.stackexchange.com/). – Sinatr Mar 16 '21 at 10:56
  • 1
    What is the intention behind the `SemaphoreContainer` class? Why not instantiate a `SemaphoreSlim` directly? – Theodor Zoulias Mar 16 '21 at 11:05
  • @TheodorZoulias this may be me missing something in the SemaphoreSlim class but I don't believe I can set the initalCount once it has been instantiated – Robert Brend Mar 16 '21 at 11:08
  • This is true, but neither you can reconfigure the `SemaphoreContainer` after it has been initialized. – Theodor Zoulias Mar 16 '21 at 12:51
  • Sorry, ignore me, I completely missed what you were asking - no particular reason other than reusability as there are a few places that I will need to do the same thing. The code above isn't exactly what I have in place but is roughly similar to demonstrate my problem – Robert Brend Mar 16 '21 at 14:10
  • I'll adjust the original code as I agree it doesn't really make as much sense right now – Robert Brend Mar 16 '21 at 14:18
  • Your use of isInitialized isn't thread-safe. – Eric J. Mar 16 '21 at 14:53
  • How about doing the initialization of the `SemaphoreSlim` in the [static constructor](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/static-constructors) of the `Handler` class? Related: [Is the C# static constructor thread safe?](https://stackoverflow.com/questions/7095/is-the-c-sharp-static-constructor-thread-safe) – Theodor Zoulias Mar 16 '21 at 15:24
  • I may be wrong but I'm not aware of any way to get the configuration value in the static constructor... If that is possible that would be my preferred solution – Robert Brend Mar 16 '21 at 17:02
  • @EricJ in the edits I made earlier I missed the ?? in the initialisation of the SemaphoreSlim class. With the new edit does it make much difference if isInitialised thread safe as the lock should prevent the pool reference from being set twice? – Robert Brend Mar 16 '21 at 17:26
  • Yeah, you are right, using the static constructor may not be as simple. I don't know much about `IConfiguration`, but I guess that there should be a neater way to configure static objects, than your current approach. Is this any helpful? [Asp.Net Core configuration in static class](https://stackoverflow.com/questions/61427798/asp-net-core-configuration-in-static-class). – Theodor Zoulias Mar 17 '21 at 10:02
  • Wouldn't be able to follow the upvoted answer as the static contructor would execute before the instance constructor so the static IConfiguration would be null at that point. And I feel like even if I got that to work that wouldn't be an improvement over my current approach, Same if not greater holes. I am starting to think that what I am trying to do may just not be worth the problems that may ensue in doing it... and that hardcoding a value that may need to change may be better than the alternatives – Robert Brend Mar 17 '21 at 13:29

1 Answers1

1

You can use Lazy to simplify your initialization code

static Lazy<SemaphoreSlim> pool = null;
public Handler(IConfiguration configuration)
{
    int poolSize = configuration.GetValue("PoolSize", 3);
    pool = new Lazy<SemaphoreSlim>(() => new SemaphoreSlim(10), LazyThreadSafetyMode.ExecutionAndPublication);
}

It works similarly to your existing code, but I always prefer to use framework methods for thread-related work when available because they are optimized by engineers who understand threading far better than the average bear.

Eric J.
  • 147,927
  • 63
  • 340
  • 553