0

this is my code that I use to ping a list of IP addresses. It works fine except for today that I received a Fatal Unhandled Exception! - System.ObjectDisposedException

private static CancellationTokenSource cts = new CancellationTokenSource();
private static CancellationToken ct;

// Source per cancellation Token
ct = cts.Token;

IsRun = true;
try
{
    LoopAndCheckPingAsync(AddressList.Select(a => a.IP).ToList()).ContinueWith((t) =>
    {
        if (t.IsFaulted)
        {
            Exception ex = t.Exception;
            while (ex is AggregateException && ex.InnerException != null)
                ex = ex.InnerException;
            Global.LOG.Log("Sonar.Start() - ContinueWith Faulted:" + ex.Message);
        }
        else
        {
            // Cancellation tokek
            if (cts != null)
            {
                cts.Dispose();          
            }
        }
    });
}
catch (Exception ex)
{
    Global.LOG.Log("Sonar.Start() - Exc:" + ex.Message);
}

As I cannot replicate the error, my suspect is related to the Disponse method of the CancellationTokenSource. Any ideas to dispose correctly a CancellationTokenSource?

I taken the Event Viewer detail entry:

Informazioni sull'eccezione: System.ObjectDisposedException
   in System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean ByRef)
   in System.StubHelpers.StubHelpers.SafeHandleAddRef(System.Runtime.InteropServices.SafeHandle, Boolean ByRef)
   in Microsoft.Win32.Win32Native.SetEvent(Microsoft.Win32.SafeHandles.SafeWaitHandle)
   in System.Threading.EventWaitHandle.Set()
   in System.Net.NetworkInformation.Ping.set_InAsyncCall(Boolean)
   in System.Net.NetworkInformation.Ping.Finish(Boolean)
   in System.Net.NetworkInformation.Ping.PingCallback(System.Object, Boolean)
   in System.Threading._ThreadPoolWaitOrTimerCallback.WaitOrTimerCallback_Context(System.Object, Boolean)
   in System.Threading._ThreadPoolWaitOrTimerCallback.WaitOrTimerCallback_Context_f(System.Object)
   in System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   in System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   in System.Threading._ThreadPoolWaitOrTimerCallback.PerformWaitOrTimerCallback(System.Object, Boolean)
BionicCode
  • 1
  • 4
  • 28
  • 44
Johnny88
  • 5
  • 7
  • Your code looks broken too. If LoopAndCheckPingAsync returns a Task (the method name implies this) you MUST await this method: `await LoopAndCheckPingAsync()`. Don't use Task.ContinueWith. Since you are awaiting the method, the code that follows the await is automatically treated as continuation. – BionicCode Dec 02 '21 at 17:19
  • You must also throw the original exception after you have logged its message. – BionicCode Dec 02 '21 at 17:22
  • @BionicCode ok I see. I changed the LoopAndCheckPingAsync method using an `await` but I don't know how to handle the cancellationToken – Johnny88 Dec 03 '21 at 07:57
  • If this is the real/complete code you must delete the complete continuation too. The await will handle exceptions properly and the existing catch will log them. The contiuation is totally redundant. Then control the CancellationTokenSource like I have suggested in my answer. I wonder why you don't pass the CancellationToken to the LoopAndCheckPingAsync method? It should have a CancellationToken parameter instead of a static reference. – BionicCode Dec 03 '21 at 09:50
  • Nice approach, I will try. Thanks a lot @BionicCode – Johnny88 Dec 09 '21 at 08:21

2 Answers2

1

It is not possible to tell where the error is originated from the code you have posted. Generally, you must inspect the eception message (callstack) to know where exactly the exception was triggered.

Call Dispose once cancellation has been requested or the cancellable operations have completed. The exception is thrown when you access the mutating members of CancellationTokenSource or its CancellationToken instance, when the CancellationTokenSource was exposed. Like when calling Cancel on the disposed instance or trying to get the reference to the associated CancellationToken after Dispose was called. You must ensure that no code accesses the disposed instance.

You can do this by setting the CancellationTokenSource property to null when disposing and by adding a null check before accessing the CancellationTokenSource. You must control the lifetime of the CancellationTokenSource carefully.

The following example shows how to control the lifetime of the CancellationTokenSource and guard against illegal referencing of the disposed instance:

private CancellationTokenSource CancellationtokenSource { get; set; }

private void CancelCancellableOperation_OnClick(object sender, EventArgs e)
{
  // Check for null to avoid an ObjectDisposedException 
  // (or a  NullReferenceException in particular) exception.
  // The implemented pattern sets the property to null immediately after disposal (not thread-safe).
  this.CancellationTokenSource?.Cancel();
}

// Start scope of CancellationTokenSource. 
// Lifetime is managed by a try-catch-finally block and the use of 
// CancellationToken.ThrowIfCancellationRequested 
// to forcefully enter the try-catch-finally block on cancellation.
private async Task DoWorkAsync()
{
  this.CancellationTokenSource = new CancellationTokenSource();
  try
  {
    await CancellableOperationAsync(this.CancellationTokenSource.Token);
  }
  catch (OperationCanceledException)
  {
    // Do some cleanup or rollback. 
    // At this point the CancellationTokenSource is still not disposed.
  }
  finally 
  {
    // Invalidate CancellationTokenSource property to raise an NullReferenceException exception 
    // to indicate that thet access ocurred uncontrolled and requires a fix.
    // Create a local copy of the property to avoid race conditions.
    var cancellationTokenSource = this.CancellationTokenSource;
    this.CancellationTokenSource = null;

    // Dispose after cancellation 
    // or cancellable operations are completed
    cancellationTokenSource.Dispose();
  }
}

private async Task CancellableOperationAsync(CancellationToken cancellationToken)
{
  // Guarantee that CancellationTokenSource is never disposed before
  // CancellationTokenSource.Cancel was called or the cancellable operation has completed

  // Do something
  while (true)
  {
    await Task.Delay(TimeSpan.FromSeconds(10));

    // Add null check if you can't guard against premature disposal
    cancellationToken?.ThrowIfCancellationRequested();
  }
}
BionicCode
  • 1
  • 4
  • 28
  • 44
  • I really like the structure of the answer you propose. I am just wondering: Why do you first set the property to null and then use another variable to dispose? Why not first call .Dispose on the property and then set the property it to null? – MiBuena Dec 30 '21 at 11:15
  • It's optional and targeting a concurrent environment. This way we can avoid race conditions in case some other thread accesses the token source property while already disposing it. You can still can cancel it multiple times though, so it doesn't provide complete thread-safety. To achieve this we need to synchronize the access to the token source, of cource. It just invalidates the token in a way that other callers will only have to care about a null reference and won't run into the situation of dealing with a ObjectDisposedException. It is not an essential part of the cancellation pattern. – BionicCode Dec 30 '21 at 11:31
  • Thank you very much! And in case we don't have multithreading in our application, then we don't need to use the additional variable, is that correct? Instead we can dispose the property directly and then set it to null? – MiBuena Dec 30 '21 at 12:00
  • Yes, exactly. That's it. – BionicCode Dec 30 '21 at 12:11
-1

Simplest solution for your problem is not to dispose of Cancellation token source.
According to MS and some posts disposal of cancellation token source is required only if it is a Linked cancellation token source or (here I'm not entirely sure) if there is something assigned by token's Register method.

quain
  • 861
  • 5
  • 18