4
\$\begingroup\$

I wrote this class to ensure that anything done in any of my worker threads can be displayed on the GUI via events, but I ran into non-thread-safe problems.

This class should take an action or a function with one or no params, i.e. the event delegate.

Is this code actually thread-safe, and, how could it be improved upon (simplifying the 4 bodies into two or one)?

The class is defined as:

private class ThreadSafeCall
{

    public void Call(Action action)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                invokable.Invoke(action, null);
            }
            else
            {
                action();
            }
        }
        catch (Exception)
        {

        }
    }

    public void Call(Action<object> action, object param)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                invokable.Invoke(action, new object[] { param });
            }
            else
            {
                action(param);
            }
        }
        catch (Exception)
        {

        }
    }

    public object Call(Func<object> action)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                return invokable.Invoke(action, null);
            }
            else
            {
                return action();
            }
        }
        catch (Exception)
        {

        }

        return null;
    }

    public object Call(Func<object, object> action, object param)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                return invokable.Invoke(action, new object[] { param });
            }
            else
            {
                return action(param);
            }
        }
        catch (Exception)
        {

        }

        return null;
    }
}

It is used:

(new ThreadSafeCall()).Call(() => { onCompleteHandle(); });
\$\endgroup\$
1
  • 7
    \$\begingroup\$ catch (Exception) { } is a bad, bad idea. \$\endgroup\$ Commented Jul 11, 2013 at 22:04

1 Answer 1

5
\$\begingroup\$

Your specific usage seems to be thread-safe. But it's relying on implementation details (about lambdas), which you shouldn't do. It's also extremely easy to get it wrong.

I believe the only way it will work is if you're closing over this, but not any other variables and that this is a type that properly implements ISynchronizeInvoke (like a WinForms form or control). This means that it won't work if:

  • you're calling it from a static method
  • you're only calling a static method from inside the lambda
  • you're closing over some local variable or parameter
  • you're calling it from another class

Because of that, I would never use code like this.

I think that you should just use Invoke() directly, something like:

Invoke(new Action(onCompleteHandle));

If you don't like that you need to specify the delegate type, you could write a couple of simple extension methods like:

public static void Invoke(this ISynchronizeInvoke target, Action action)
{
    target.Invoke(action, null);
}

and then use it like this (that this is required):

this.Invoke(onCompleteHandle);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.