1
\$\begingroup\$

I am trying to throttle SemaphoreSlim (i.e. allow initialization with a negative initialCount). A scenario could be that you're hitting an API and may notice degradation due to a server overload, so you'd want to start throttling requests to, say, 10 concurrent requests. However, you would be keeping count of the concurrent requests and know that at this time there are 25 concurrent requests, but you can't start a SemaphoreSlim(-15, 10).

I tried making an implementation that allows this, but I'm not 100% sure whether or not this is thread-safe and if it could be optimized (e.g. doing without the locks).

public class SemaphoreSlimThrottle : SemaphoreSlim
{
    private volatile int _throttleCount;
    private readonly object _lock = new object();

    public SemaphoreSlimThrottle(int initialCount)
        : base(initialCount)
    {
    }

    public SemaphoreSlimThrottle(int initialCount, int maxCount)
        : base(Math.Max(0, initialCount), maxCount)
    {
        _throttleCount = Math.Min(0, initialCount);
    }

    public new int CurrentCount => _throttleCount + base.CurrentCount;

    public new int Release()
    {
        if (_throttleCount < 0)
        {
            lock (_lock)
            {
                if (_throttleCount < 0)
                {
                    _throttleCount++;
                    return _throttleCount - 1;
                }
            }
        }
        return base.Release();
    }

    public new int Release(int releaseCount)
    {
        if (releaseCount < 1)
        {
            base.Release(releaseCount); // throws exception
        }

        if (releaseCount + _throttleCount <= 0)
        {
            lock (_lock)
            {
                if (releaseCount + _throttleCount <= 0)
                {
                    _throttleCount += releaseCount;
                    return _throttleCount - releaseCount;
                }
            }
        }

        if (_throttleCount < 0)
        {
            lock (_lock)
            {
                if (_throttleCount < 0)
                {
                    int output = CurrentCount;
                    base.Release(releaseCount + _throttleCount);
                    _throttleCount = 0;
                    return output;
                }
            }
        }

        return base.Release(releaseCount);
    }
}

I've packaged this as a NuGet package with source available on GitHub.

\$\endgroup\$
2
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Apr 16, 2022 at 10:33
  • \$\begingroup\$ Feel free to post a new question with updated code. If you decide to post it as an answer instead, make sure the answer contains why the new code is better and how it's better. \$\endgroup\$
    – Mast
    Apr 16, 2022 at 10:35

1 Answer 1

1
\$\begingroup\$

First the "new" keyword. If casting this class back down to SemaphoreSlim then none of the code that has the new keyword would be used and just the base SemaphoreSlim code would execute.

Since we want the new code to execute I would suggest not inheriting from SemaphoreSlim but rather wrap it and then chain the calls down into the wrapped SemaphoreSlim. Something like, I didn't put in all the method this just an example of the approach.

public class SemaphoreSlimThrottle : IDisposable
{
    private volatile int _throttleCount;
    private readonly SemaphoreSlim _semaphore;
    private readonly object _lock = new object();
    private bool _turnOffThrottleCheck = false;

    public SemaphoreSlimThrottle(int initialCount, int maxCount)
    {
        if (initialCount < 0)
        {
            _semaphore = new SemaphoreSlim(0, maxCount);
            _turnOffThrottleCheck = true;
        }
        else
        {
            _semaphore = new SemaphoreSlim(initialCount, maxCount);
            _turnOffThrottleCheck = true;
        }
    }

    public WaitHandle AvailableWaitHandle => _semaphore.AvailableWaitHandle;

    public int CurrentCount => _throttleCount + _semaphore.CurrentCount;

    public bool Wait(TimeSpan timeout, CancellationToken cancellationToken) => _semaphore.Wait(timeout, cancellationToken);

    public int Release() => Release(1);

    public void Dispose()
    {
        _semaphore.Dispose();
    }
}

The other optimization that can be preformed is to not have the volatile field accessed once the "extra" have been used up. Also calling into the base outside the lock to have the locks short as possible.

Warning I did not test this code there could be typo or bugs in it - use as a gist

    public int Release(int releaseCount)
    {
        // using bool property to avoid unnecessary volatile accesses in happy path
        if (releaseCount < 1 || _turnOffThrottleCheck)
        {
            return _semaphore.Release(releaseCount);
        }

        int remainingCount;
        var returnCount = 0;
        lock (_lock)
        {
            var throttleCount = _throttleCount;
            if (throttleCount == 0) // Different thread release all them just call into base
            {
                remainingCount = releaseCount;
            }
            else if (releaseCount + throttleCount < 0) // Releasing less than throttle just decrease
            {
                _throttleCount += releaseCount;
                remainingCount = 0;
                returnCount = throttleCount;
            }
            else // releasing all the throttles
            {
                _throttleCount = 0;
                _turnOffThrottleCheck = true;
                returnCount = throttleCount;
                remainingCount = releaseCount + throttleCount;
            }
        }

        // doing outside lock
        if (remainingCount > 0) // call into base if more locks to be released
        {
            return _semaphore.Release(releaseCount) + returnCount;
        }

        return returnCount + _semaphore.CurrentCount;

    }

This way we don't access _throttleCount in the happy path. We also release the lock and just call into the SemaphoreSlim outside the lock.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your comments. Re the inheritance, I understand, but if you are casting down to SemaphoreSlim I'm assuming you specifically don't want the features. It's cleaner inheriting from SemaphoreSlim: if new methods are added, these will be automatically available. As for the rest, I like the idea. The lock is only used in the unhappy path where it's not really releasing anything. I'm curious as to why you're reading _throttleCount into var throttleCount given we're operating inside a lock. \$\endgroup\$ Apr 16, 2022 at 5:44
  • \$\begingroup\$ One other thing, given we're using volatile, is this running the risk of a lock being released and another thread acquiring it and reading _throttleCount before it's synchronized to the latest value? Because that would mean ending up with a negative _throttleCount. \$\endgroup\$ Apr 16, 2022 at 6:00
  • 1
    \$\begingroup\$ Also, in the same way, would this make sense? public new int CurrentCount => _throttleCount + _semaphore.CurrentCount; to: public new int CurrentCount => (!_throttleEnabled) ? base.CurrentCount : _throttleCount + base.CurrentCount; \$\endgroup\$ Apr 16, 2022 at 6:27
  • 1
    \$\begingroup\$ Now I see what you mean with regards to casting down, as you could pass it on to a method accepting SemaphoreSlim. \$\endgroup\$ Apr 16, 2022 at 6:30
  • 1
    \$\begingroup\$ I did take a look at your git and looks pretty good. CodeReview site isn't good to just have a link to a git repo as things can change in the future and the repo might not be there anymore. but for a comment I think it's ok. If you wanted a way to get the root SemaphoreSlim from the class you could make a property of Task<SemaphoreSlim> and a taskcompletionsource that completes the Task once the throttle is hit. I don't know if it adds much value just if you did need some way to pass it down to a SemaphoreSlim could await until the "extras" are released. \$\endgroup\$ Apr 16, 2022 at 23:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.