SemaphoreSlim problems with async tasks

I've been attempting to use SemaphoreSlim to limit the amount of concurrent tasks I have running at any one time but it seems to have no effect, likely down to my implementation which is why I'm here. My SemaphoreSlim code is like so:

First it's called by

await Task.Run(() => mc.StartAsync());

Calling this method

public async Task StartAsync()
{
    using (SemaphoreSlim concurrencySemaphore = new SemaphoreSlim(5))
    {
        foreach (var task in ThreadHandler.ThreadList)
        {
            await concurrencySemaphore.WaitAsync();
            try
            {
                await Task.Run(() => task.Operation.Start());
            }
            finally
            {
                concurrencySemaphore.Release();
            }
        }
    }
}

This in turn is starting a task from a list which looks like this, this is in a custom model with the task stored and created previously

public Task Operation { get; set; }

var t = new Task(async () =>
{
    await Task.Run(() => Method(input, sps));
});

Remembering my code doesn't work as I'd expect, is this the correct way to start something like this? I don't expect 3 tasks launching from a single point is a good idea. The main reason It's like this is because I'm executing an Action<> and couldn't figure out a way to await it alone. Do these tasks count towards the SemaphoreSlim limit?

After various tests I can confirm that my SemaphoreSlim code is just continuously executing the tasks, I added a large task delay into the list of tasks to see if I could stop it from executing which worked but new tasks were still launched... what am I missing?

My goal is to have a limit on the number of tasks concurrently running, if that wasn't clear. Thank you for any assistance!

EDIT: I think I've realised I'm only awaiting the starting of the task, not the completion.

Solution

I think I've realised I'm only awaiting the starting of the task, not the completion.

Indeed, that is the core of the problem.

You shouldn't use the Task constructor, ever, at all, for anything. Just pretend it doesn't exist. It will always lead you down an awkward path.

If you have an action you want to perform at a later time, you should use a delegate: Action or Func<T> for synchronous work, and Func<Task> or Func<Task<T>> for asynchronous work. E.g., if Method is synchronous, then you would have:

public Action Operation { get; set; }
...
Operation = () => Method(input, sps);

Then you can invoke it using Task.Run as such:

public async Task ProcessAsync()
{
    using (SemaphoreSlim concurrencySemaphore = new SemaphoreSlim(5))
    {
        var tasks = ThreadHandler.ThreadList.Select(async task =>
        {
            await concurrencySemaphore.WaitAsync();
            try
            {
                await Task.Run(() => task.Operation());
            }
            finally
            {
                concurrencySemaphore.Release();
            }
        }).ToList();
        await Task.WhenAll(tasks);
    }
}

The above code will work fine if Operation is Action (synchronous) or Func<Task> (asynchronous).

However, if it is Action (i.e., synchronous), then what you're really doing is parallel processing, not asynchronous concurrency, and there's built-in types that can help with that:

public void Process()
{
    // Only valid if Operation is Action, not Func<Task>!
    Parallel.ForEach(
        ThreadHandler.ThreadList,
        new ParallelOptions { MaxDegreeOfParallelism = 5 },
        task => task.Operation());
}