CODEWITHSUNDEEP
c#thread-safetytimeoutinvoke
Rich175
Rich175

Reputation: 3

C# timeout - is mine dangerous...?

I have created a timeout function based on things I have seen in various places but am pretty sure I am not doing it a great way! (But it does seem to work.)

I am connecting to a piece of hardware that if working connects in a few seconds but if not takes around 1 minute to timeout. So if I can create my own timeout function I can set it at 20 seconds and save lots of time and waiting.

I have tried to make it so my timeout returns a string:

static string CallWithTimeout(Action action, int timeoutMilliseconds)
{
    string reply = "";
    Thread threadToKill = null;
    Action wrappedAction = () =>
    {
        threadToKill = Thread.CurrentThread;
        action();
    };

    IAsyncResult result = wrappedAction.BeginInvoke(null, null);
    if (result.AsyncWaitHandle.WaitOne(timeoutMilliseconds))
    {
        reply = "Connected";
        wrappedAction.EndInvoke(result);
        return reply;
    }
    else
    {
        threadToKill.Abort();
        reply = "Error";
        return reply;            
    }      
}

then I call it with something like :

string replyfromreader = CallWithTimeout(connectToHardware, 20000);

the connectToHardware is just a one liner so no need to post.

Upvotes: 0

Views: 471

Answers (2)

Richard
Richard

Reputation: 109025

Avoiding Thread.Abort is always a good idea. Avoiding it on a thread you did not create is even better.

Assuming if the hardware is not working, and you want the timeout, it does not matter if connectToHardware is left to timeout on its own and no error/exception details are wanted, then you can use the Task Parallel Library (TPL): System.Threading.Tasks.Task:

// True => worked, False => timeout
public static bool CallWithTimeout(Action method, TimeSpan timeout) {
  Exception e;
  Task worker = Task.Factory.StartNew(method)
                    .ContineueWith(t => {
                      // Ensure any exception is observed, is no-op if no exception.
                      // Using closure to help avoid this being optimised out.
                      e = t.Exception;
                    });
  return worker.Wait(timeout);
}

(If the passed Action could interact with a passed CancellationToken this could be made cleaner, allowing the underlying method to fail quickly on timeout.)

Upvotes: 0

Hans Passant
Hans Passant

Reputation: 941625

It's okayish as far as .NET state is concerned. You won't call EndInvoke(), that leaks resources for 10 minutes, the default lifetime of remoted objects.

In a case like this, calling Thread.Abort() has a very small chance of succeeding. A managed thread needs to be in an alertable wait state to be abortable, it just never is when the thread is buried deep inside native code that ultimately waits for some device driver call to complete.

Leaving the CLR in a state where it keeps trying to abort a thread and never succeeds is not particularly pleasant, not something I've ever tried on purpose so no real idea what the side-effects are. It does however mean that your code will block on the Abort() method call so you still haven't fixed the problem. The best thing to do is therefore to not abort the thread but just abandon it. Setting a flag that marks the device dead so you don't try to do this ever again.

If you want to continue running your program, even without the device being in a usable state, and you want to provide a way to recover from the problem then you'll need an entirely different approach. You'll need to put the device related code in a separate process. Which you can then Kill() when the device is unresponsive, relying on Windows to clean up the shrapnel. Interop with that process using a low-level mechanism like named pipes or sockets is best so you can recover from the disconnect fairly easily.

Upvotes: 1

Related Questions