I created a following class to manage multithreading without extra overhead, which exist when I use Parallel
TPL class. It is also useful for systems without TPL
.
Can it be enchanced?
public static void Foreach<T>(this ICollection<T> source, Action<T> action)
{
var allDone = new ManualResetEventSlim(false);
int completed = 0;
foreach (var item in source)
{
ThreadPool.QueueUserWorkItem(state =>
{
var closure = (T)state;
action(closure);
if (Interlocked.Increment(ref completed) == source.Count)
allDone.Set();
}, item);
}
allDone.Wait();
}
public static void Do(Action action1, Action action2)
{
var firstTask = QueueWaitableTask(action1);
action2();
firstTask.Wait();
}
public static ManualResetEventSlim QueueWaitableTask(Action action)
{
var result = new ManualResetEventSlim(false);
ThreadPool.QueueUserWorkItem(mse =>
{
action();
((ManualResetEventSlim) mse).Set();
}, result);
return result;
}
-
\$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . Feel free to ask a follow-up question if sufficient changes have been made. \$\endgroup\$Mast– Mast ♦2016年02月29日 16:53:18 +00:00Commented Feb 29, 2016 at 16:53
-
\$\begingroup\$ @Mast this is why I didn't replace my original code, but wrote a new version. But thanks for a link, I will check it. \$\endgroup\$Alex Zhukovskiy– Alex Zhukovskiy2016年02月29日 16:55:59 +00:00Commented Feb 29, 2016 at 16:55
-
\$\begingroup\$ I updated with a comment about your other version of the code. \$\endgroup\$RobH– RobH2016年02月29日 17:47:39 +00:00Commented Feb 29, 2016 at 17:47
1 Answer 1
You should consider what happens when a user passes an action which can error:
new List<string> { "a", "b", "c" }.Foreach(_ => { throw new Exception(); });
In this case, your manual reset event will never be signalled and the code will never finish executing. Eeek!
As ManualResetEventSlim
was introduced in .Net 4 you may as well use another class that was new at the same time CountdownEvent
.
public static void Foreach<T>(this ICollection<T> source, Action<T> action)
{
var allDone = new CountdownEvent(source.Count);
foreach (var item in source)
{
ThreadPool.QueueUserWorkItem(state =>
{
try
{
action((T)state);
}
finally
{
allDone.Signal();
}
}, item);
}
allDone.Wait();
}
Having said that Foreach
is a bad name, it's too similar to List.ForEach
and doesn't even hint at the internal behaviour.
You should be checking that source != null
and action != null
.
Update:
If you want to catch exceptions and rethrow them as an AggregateException, add them to a concurrent collection e.g. ConcurrentBag
so you don't need to worry about synchronisation.