0
\$\begingroup\$

I have a task wrapper which stores tasks, their CancellationTokens and of course their names (botName). botName is important, because when I want to stop a bot/cancel task, I will have to find it in the collection.

I wonder if there is a better collection to do that, because it looks ugly, especially the CancellationToken cancellation logic.

public class BotWrapper
{
    private readonly static ConcurrentDictionary<string, Tuple<Task, CancellationTokenSource>> _bots = new ConcurrentDictionary<string, Tuple<Task, CancellationTokenSource>>();

    public void Start(string botName, string symbol, KlineInterval interval, StrategyBase strategy)
    {
        CancellationTokenSource cts = new CancellationTokenSource();

        Task task = Task.Run(() => strategy.Start(symbol, interval, cts.Token), cts.Token);

        _bots.TryAdd(botName, new Tuple<Task, CancellationTokenSource>(task, cts));
    }

    public void Stop(string botName)
    {
        foreach (var bot in _bots)
        {
            if (bot.Key.Contains(botName))
            {
                CancellationTokenSource cts = bot.Value.Item2;
                cts.Cancel();

                Thread.Sleep(2500);

                cts.Dispose(); // unmanaged

                _bots.TryRemove(botName, out _);
            }
        }
    }
}

By the way StrategyBase.Start is nothing specific:

public virtual void Start(string symbol, KlineInterval interval, CancellationToken token)
{
    ... logic

    ... and at some point

    if (token.IsCancellationRequested)
    {
        try
        {
            token.ThrowIfCancellationRequested();
        }
        catch (OperationCanceledException)
        {
            SocketClient.UnsubscribeAll();
            Console.WriteLine("Task canceled.");
        }
    }
}
```
\$\endgroup\$
1
  • \$\begingroup\$ Have you considered to use TaskCompletionSource along with CancellationTokenSource? \$\endgroup\$ May 15 '20 at 9:21
5
\$\begingroup\$

Duplicate logic

if (token.IsCancellationRequested)
{
    try
    {
        token.ThrowIfCancellationRequested();
    }
    catch (OperationCanceledException)
    {
        SocketClient.UnsubscribeAll();
        Console.WriteLine("Task canceled.");
    }
}

This code is weird. You first check if a cancellation is requested, then you ask for an exception to be throw if a cancellation is requested (which you already know it is), and then you immediately catch that exception. It seems like you've conflated two ways of doing the same thing. You could instead use either:

try
{
    token.ThrowIfCancellationRequested();
}
catch (OperationCanceledException)
{
    SocketClient.UnsubscribeAll();
    Console.WriteLine("Task canceled.");
}

OR

if (token.IsCancellationRequested)
{
    SocketClient.UnsubscribeAll();
    Console.WriteLine("Task canceled.");
}

Which approach you use depends on whether it's meaningful to throw an exception, and it doesn't seem meaningful here (though you have actually omitted the surrounding code so I can't give you a guarantee).

Throwing an exception is an expensive operation, and its main purpose is to bubble an exception up through multiple layers without requiring manual handling at every step. Since you're throwing and catching at the same layer, the cost of the expensive throw is not worth the benefit.

Based on what I see, a simple if suffices here.


Tuples

I don't like tuples (in this particular scenario, at least). They're a honeytrap.

At first sight, they look really easy by allowing you to skip creating a custom class, but then you are stuck using unreadable Item1, Item2, ... properties which have no meaning. In any reasonably sized codebase, I'd have to look up which item is stored in which slot several times during the development cycle.

I suggest replacing this with a custom class just so you can keep the property names relevant:

public class BotTask
{
    public Task Task { get; private set; }
    public CancellationTokenSource CancellationTokenSource { get; private set; }

    public BotTask (Task task, CancellationTokenSource cancellationTokenSource)
    {
        this.Task = task;
        this.CancellationTokenSource = cancellationTokenSource;
    }
}

It's a few lines extra, but it significantly improves readability in the code that uses these objects, e.g.:

// Previously

CancellationTokenSource cts = bot.Value.Item2;
cts.Cancel();

// Now

bot.Value.CancellationTokenSource.Cancel();

The increased readability and somewhat shortened line count in the usage generally outweighs the effort of writing a simple DTO class.


Contains

It seems to me that you're conflating the Dictionary.ContainsKey and String.Contains methods:

public void Stop(string botName)
{
    foreach (var bot in _bots)
    {
        if (bot.Key.Contains(botName))
        {
            // ...
        }
    }
}

If you have a list of botTasks with names Alice, Bob, Cindy and you ask to stop the both with name ind, then you should come up dry since no bot with that exact name exists. However, in your current logic, that is not the case, since "Cindy".Contains("ind") returns true!

What you presumably want is to check if the dictionary contains the key "ind" (exactly), instead of whether the dictionary contains any key which contains "ind" in part of the key. That can be achieved like so:

public void Stop(string botName)
{
    if(_bots.ContainsKey(botName))
    {
        var bot = _bots[botName];

        // ...
    }
}

Or alternatively:

public void Stop(string botName)
{
    if(_bots.TryGetValue(botName, out bot))
    {
        // ...
    }
}

Thread.Sleep

Thread.Sleep(2500);

Since you're using async in your code, it seems counterproductive to then use Thread.Sleep instead of the asynchronous ´Task.Delay´, i.e.:

await Task.Delay(2500);

This is a tiny aside, because I'll omit this delay in the next section, but it's still important to point out in case a delay was actually needed here.


Manual disposing

I think you can skip the "sleep and dispose" logic here.

It's good to see that you are managing your resources responsibly, but in this particular case the management is causing an undesirable blocking of the thread for 2.5 seconds. That's not good. Even if you use an await Task.Delay instead, I'm still not convinced that it's necessary to manually handle this.

When nothing references the cancellation token source anymore, the garbage collector will eventually pick this up and dispose of the object itself. There are cases where you don't have the luxury of waiting on the garbage collector to kick into action, but in such cases blocking the thread for 2.5 seconds would still be a bigger evil.

You can simplify the logic:

public void Stop(string botName)
{
    if(_bots.ContainsKey(botName))
    {
        var bot = _bots[botName];

        bot.Value.CancellationTokenSource.Cancel();

        _bots.TryRemove(botName, out _);
    }
}

Instead of waiting for the task to cancel itself, your code instead entrusts that the task will eventually end itself, and your code only focuses on getting its own affairs in order, i.e. removing the task from your dictionary.


Static ConcurrentDictionary

This is more of an educated guess than a guarantee.

Your static ConcurrentDictionary comes across as a red flag to me, where you're misusing statics.

It seems to me like you're intending to spawn multiple BotWrapper instances which you expect to share the same static botlist, but that's a flawed approach. It would be better if you instead pass the same BotWrapper instance around in your codebase (whether through DI containers or manually, I don't know how your architecture is set up), and then keep the botlist as a non-static property of that instance.

The benefits of managing a single instance are a minor memory improvement (since you don't need multiple instances anymore), and it allows you to scale out your codebase in case you ever need to manage two different lists of bottasks. Using statics, it's impossible to have more than one. While it may not be necessary right now to maintain different lists, never say never, and it's good practice to allow for maximum reusability down the line.

However, you didn't actually provide enough code for me to make a final decision on this. This is just an educated guess based on the code as it is presented.

\$\endgroup\$
2
  • \$\begingroup\$ First of all, thank you, I didn't even think that I could get a such incredible answer! Nice example for the exceptions. It's indeed unnecessary to throw an exception and then catch it in same layer. You were right about CancellationToken's disposal (stackoverflow.com/questions/6960520/… @Bryan Crosby). I thought it was unmanaged, but it seems people actually checked it with dnSpy/ILSpy. \$\endgroup\$
    – nop
    May 15 '20 at 10:41
  • \$\begingroup\$ Continuing the comment, because it was too long. I will actually have only one instance of BotWrapper. I didn't misuse static at first point, because I had other thoughts (because I remember how Hangfire was). \$\endgroup\$
    – nop
    May 15 '20 at 10:52

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.