11
\$\begingroup\$

I want to throttle asynchronous actions at a specific maximum rate of n actions per t period, and have pending actions wait until there is a free slot (rather than drop them). I want to receive the outcome When an action completes or fails. I also want the option of cancelling an action. The order of actions is unimportant, though I have been considering if a priority queue would be worth the trouble.

For example, if your maxActions was 2 and your maxPeriod 1 second, if 3 actions each taking 500 ms were queued simultaneously then the first two would run immediately and complete in 500ms but the 3rd would have to wait a whole second before starting, rather than starting at 500ms.

What I've come up with so far is based around two semaphores, one to control the number of actions active at any one time, and one to control the number of actions throughout the max period. I release separately, because it is possible that a request may take longer than the given period.

Are there any logical errors to this approach (e.g., could a semaphore potentially never be released)? Is there a completely different approach that would be better?

public class Throttle
{
    private readonly TimeSpan _maxPeriod;
    private readonly System.Threading.SemaphoreSlim _throttleActions, _throttlePeriods;

    public Throttle(int maxActions, TimeSpan maxPeriod)
    {
        _throttleActions = new System.Threading.SemaphoreSlim(maxActions, maxActions);
        _throttlePeriods = new System.Threading.SemaphoreSlim(maxActions, maxActions);
        _maxPeriod = maxPeriod;
    }

    public Task<T> Queue<T>(Func<T> action, System.Threading.CancellationToken cancel)
    {
        return _throttleActions.WaitAsync(cancel).ContinueWith<T>(t =>
        {
            try
            {
                _throttlePeriods.Wait(cancel);

                // Release after period
                // - Allow bursts up to maxActions requests at once
                // - Do not allow more than maxActions requests per period
                Task.Delay(_maxPeriod).ContinueWith((tt) =>
                {
                    _throttlePeriods.Release(1);
                });

                return action();
            }
            finally
            {
                _throttleActions.Release(1);
            }
        });
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ If your goal is to limit number of simultaneous actions, maybe something like LimitedConcurrencyTaskScheduler would be more appropriate solution? (like msdn.microsoft.com/en-us/library/ee789351(v=vs.110).aspx) \$\endgroup\$ – Sergey Teplyakov Apr 17 '15 at 0:54
  • \$\begingroup\$ It would be nice to include a small piece of client code consuming this class, for illustrative purposes. \$\endgroup\$ – Mathieu Guindon Apr 17 '15 at 1:25
  • \$\begingroup\$ @SergeyTeplyakov Thanks for your suggestion - I've looked over LimitedConcurrencyTaskScheduler briefly; the goal of my code is specifically limit the number of connections in a period (as well as the number of concurrent connections), it appears that the scheduler example is unbounded for the number of requests per second? If so, it's not usable for my purposes \$\endgroup\$ – MatthewToday Apr 17 '15 at 2:57
  • \$\begingroup\$ @Mat'sMug I'll put an small example together when I've got the time later, but it won't be much more than a few calls to Queue with some simulated work to demonstrate the throttling behaviour of actions per second. \$\endgroup\$ – MatthewToday Apr 17 '15 at 2:59
6
\$\begingroup\$
private readonly TimeSpan _maxPeriod;
private readonly System.Threading.SemaphoreSlim _throttleActions, _throttlePeriods;

I would add using System.Threading; at the top of the code file, so as to avoid having to fully qualify everything you're referencing in that namespace.

Also, at a glance (especially with the fully qualified SemaphoreSlim) it looks like you're declaring two readonly fields here, but there's really 3. I find this requires less brain juice to glance at:

private readonly TimeSpan _maxPeriod;
private readonly SemaphoreSlim _throttleActions;
private readonly SemaphoreSlim _throttlePeriods;

That said I like that the fields are readonly and that you're using an underscore prefix, which avoids having to qualify the fields with this in the constructor:

public Throttle(int maxActions, TimeSpan maxPeriod)
{
    this.throttleActions = new SemaphoreSlim(maxActions, maxActions);
    this.throttlePeriods = new SemaphoreSlim(maxActions, maxActions);
    this.maxPeriod = maxPeriod;
}

...and the reason you would have to qualify the fields in the constructor without that underscore prefix, is because you have a consistent naming scheme for your constructor parameters vs. the fields they're assigning, and that's excellent.

I'm not very familiar with asynchronous programming, multithreading or even the TPL - but Throttle tickles my Name-O-Meter™, as it's a verb more than a noun.. and class names should be nouns. Would Throttler be weird for a class like this? The only time a verb seems fine to me is when the class is static, such as Convert.

public Task<T> Queue<T>(Func<T> action, System.Threading.CancellationToken cancel)

Here we have the opposite situation: you're using a noun where a verb would be more appropriate - Enqueue<T> sounds more like it.

I'm on the fence about these comments:

    // Release after period
    // - Allow bursts up to maxActions requests at once
    // - Do not allow more than maxActions requests per period

It seems to me that they wouldn't be needed if the constructor had XML comments for its parameters:

/// <summary>
/// Instantiates a new <c>Throttle</c> object
/// </summary>
/// <param name="maxActions">The maximum number of actions allowed per period.</param>
/// <param name="maxPeriod">The time span to use for delaying tasks.</param>
public Throttle(int maxActions, TimeSpan maxPeriod)
{
    this.throttleActions = new SemaphoreSlim(maxActions, maxActions);
    this.throttlePeriods = new SemaphoreSlim(maxActions, maxActions);
    this.maxPeriod = maxPeriod;
}

I've never used Task.Delay, so I'm not 100% on the actual meaning of maxPeriod - but you get the idea: not only XML documentation cleans up comments in the middle of your implementation, it's also picked up by IntelliSense and shown when you're writing the client code.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I appreciate the style comments, though I'm more looking for specific comments on the logic - hidden pitfalls of say, Task.Delay with Semaphores, or behaviour of WaitAsync that I'm not familiar with, that would render this technique faulty. \$\endgroup\$ – MatthewToday Apr 17 '15 at 3:01
  • \$\begingroup\$ MaxPeriod is probably poorly named, but it represents the period in which only maxActions may occur. For example, if your maxActions was 2 and your maxPeriod 1 second, if 3 actions each taking 500 ms were queued simultaneously then the first two would run immediately and complete in 500ms but the 3rd would have to wait a whole second before starting, rather than starting at 500ms. \$\endgroup\$ – MatthewToday Apr 17 '15 at 3:06
  • \$\begingroup\$ I don't see anything wrong with your approach, it looks pretty sound to me - but then again, I know that I don't know enough on the subject to mention anything like that in an answer and risk sticking a foot in my mouth... I'm sure another C# reviewer that's more familiar with that part of the framework than I am will come along and address that aspect of the code though - I just had to mention these points anyway. \$\endgroup\$ – Mathieu Guindon Apr 17 '15 at 3:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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