HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Invoking worker threads

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
threadsinvokingworker

Problem

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 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 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 action, object param)
{
try
{
ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
if (invokable != null && invokable.InvokeRequire

Solution

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);

Code Snippets

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

Context

StackExchange Code Review Q#28384, answer score: 5

Revisions (0)

No revisions yet.