Reliable delayed execution class in C#

Question

To control delays between animations in an iOS game I've developed I have written this "Delayer" class below. But I'm experiencing some rare random crashes possible related to objects being deallocated:

0x102933344 - /var/containers/Bundle/Application/52DE96A6-70CF-4D3D-A6F0-3DDAB4F31347/DiscDrop.iOS.app/DiscDrop.iOS : mono_dump_native_crash_info
0x1029295b0 - /var/containers/Bundle/Application/52DE96A6-70CF-4D3D-A6F0-3DDAB4F31347/DiscDrop.iOS.app/DiscDrop.iOS : mono_handle_native_crash
0x10293776c - /var/containers/Bundle/Application/52DE96A6-70CF-4D3D-A6F0-3DDAB4F31347/DiscDrop.iOS.app/DiscDrop.iOS : mono_sigsegv_signal_handler_debug
0x1dd9f69fc - /usr/lib/system/libsystem_platform.dylib : <redacted>
0x1dcfd9b9c - /usr/lib/libobjc.A.dylib : <redacted>
0x1dcfd9b9c - /usr/lib/libobjc.A.dylib : <redacted>
0x1dddf7bb0 - /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation : _CFAutoreleasePoolPop
0x1de871744 - /System/Library/Frameworks/Foundation.framework/Foundation : <redacted>

so I am wondering if anyone can spot a problem with the class:

public class Delayer
{
    private readonly List<CancellationTokenSource> cancellationTokenSources;

    public Delayer()
    {
        this.cancellationTokenSources = new List<CancellationTokenSource>();
    }

    public void DelayedCall(float delay, Action callback)
    {
        CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
        this.cancellationTokenSources.Add(cancellationTokenSource);
        _ = Delayer.CallbackAfterDelay(delay, cancellationTokenSource, delegate
        {
            this.cancellationTokenSources.Remove(cancellationTokenSource);
            callback?.Invoke();
        });
    }

    public void CancelAll()
    {
        foreach (CancellationTokenSource cancellationTokenSource in this.cancellationTokenSources)
        {
            cancellationTokenSource.Cancel();   // cancellationTokenSource.Dispose(); here doesnt help leak
        }
        this.cancellationTokenSources.Clear();
    }

    private static async Task CallbackAfterDelay(float delay, CancellationTokenSource cancellationTokenSource, Action callback)
    {
        await Task.Delay((int)(delay * 1000), cancellationTokenSource.Token);
        if (cancellationTokenSource.IsCancellationRequested)
        {
            return;
        }
        try
        {
            callback?.Invoke();
        }
        catch (Exception exception)
        {
            System.Diagnostics.Debug.WriteLine("### Common.Delayer.DelayedCall caught exception={0}", exception.Message);
            throw;
        }
    }
}

This is an example of how I use it:

this.delayer = new Delayer();
//...
this.delayer.DelayedCall(delay: 0.5f, callback: delegate
{
    this.PlaySound(duration: 0.2f);
}

Note also that your class is not thread safe, and I'm taking a guess here that (based on the design) you intend to call methods on this class concurrently. If that's the case, adding and removing from the list is not safe without a locking mechanism — pinkfloydx33, Feb 03 '21 at 13:06
@pinkfloydx33 the OP is probably intending to use this class in a GUI application with a `SynchronizationContext` installed. In that case the `SynchronizationContext` will take care of synchronizing all asynchronous continuations, by invoking them on the UI thread. Otherwise things would be much more complex. You can see [here](https://stackoverflow.com/questions/6960520/when-to-dispose-cancellationtokensource/61681938#61681938) a thread-safe `CancelableExecution` class, and how much code I had to write in order to make it work correctly. — Theodor Zoulias, Feb 03 '21 at 16:21
@TheodorZoulias I'm talking about access to the `cancellationTokenSources` list. A SyncContext isnt going to synchronize access to that field. If two Adds or an Add/remove happen simultaneously bad things can happen. Better to use a lock around access or a concurrent collection. Just saying — pinkfloydx33, Feb 03 '21 at 16:24
@pinkfloydx33 how is it possible for two simultaneous Adds (or an Add/remove) to occur, if all code paths are going to run on the same thread? — Theodor Zoulias, Feb 03 '21 at 16:28
@TheodorZoulias that's assuming they are. Back to my original comment was "*if*" they had concurrent access to be careful. Not that they *do* — pinkfloydx33, Feb 03 '21 at 16:30
@pinkfloydx33 well, lets wait for the OP to clarify whether the `Delayer` class is supposed to be thread-safe or not. — Theodor Zoulias, Feb 03 '21 at 16:34
Delayer is absolutely only ever intended to be called on the main UI thread, so no need to worry about concurrent access. I should have mentioned that in the question, sorry. — Bbx, Feb 04 '21 at 13:11

score 0 · Answer 1 · answered Feb 03 '21 at 12:50

0

The _ = Delayer.CallbackAfterDelay task is launched in a fire-and-forget fashion, so you won't be notified in case there are any bugs in your implementation. And I can see a bug already: awaiting the Task.Delay may result to an OperationCanceledException which is not caught, so in this case the callback?.Invoke(); will not be invoked, resulting to the associated cancellationTokenSource not being removed from the list.

The easiest way to ensure that all exceptions will be surfaced is to convert the CallbackAfterDelay from async Task to async void. This change will force you to be very careful with how you code things, because any non-caught exception inside this method will be unhandled (not simply unobserved), and will crash the process.

answered Feb 03 '21 at 12:50

Theodor Zoulias

34,835
7
69
104

1

Thanks for the quick reply. What causes OperationCanceledException? Do you mean when cancellationTokenSource.Cancel() is called? I think that's the only way it can happen. In which case I don't want the callback to be called (I want CancelAll to stop any pending callbacks). And yes I am missing exceptions being caught so would be good to fix that. – Bbx Feb 03 '21 at 13:12
@Bbx yes, the task returned by `Task.Delay` may complete in a [`Canceled`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskstatus) state, in case the supplied `CancellationToken` is canceled before the task's completion. And when awaiting canceled tasks, an `OperationCanceledException` is thrown. – Theodor Zoulias Feb 03 '21 at 13:16

Reliable delayed execution class in C#

1 Answers1