5

I've tried this...

public ArrayList GetAllObjectAttributes()
    {
        List<Task> taskList = new List<Task>();
        ArrayList allObjectAttributes = new ArrayList();
        taskList.Add(Task.Factory.StartNew(() => { allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Folder));}));
        taskList.Add(Task.Factory.StartNew(() => { allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.XMLFile)); }));
        taskList.Add(Task.Factory.StartNew(() => { allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.TextFile)); }));
        taskList.Add(Task.Factory.StartNew(() => { allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Parent)); }));
        Task.WaitAll(taskList.ToArray());
        return allObjectAttributes;
    }

and this...

public ArrayList GetAllObjectAttributes()
    {
        Thread[] threads = new Thread[4];
        ArrayList allObjectAttributes = new ArrayList();
        threads[0] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Folder)));
        threads[1] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.XMLFile)));
        threads[2] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.TextFile)));
        threads[3] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Parent)));

        foreach(Thread thread in threads)
        {
            thread.Start();
            thread.Join();
        }
        return allObjectAttributes;
    }

and this too...

public ArrayList GetAllObjectAttributes()
    {
        Thread[] threads = new Thread[4];
        ArrayList allObjectAttributes = new ArrayList();
        threads[0] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Folder)));
        threads[1] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.XMLFile)));
        threads[2] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.TextFile)));
        threads[3] = new Thread(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Parent)));

        foreach(Thread thread in threads)
        {
            thread.Start();
        }
        while(threads[0].IsAlive || threads[1].IsAlive || threads[2].IsAlive || threads[3].IsAlive)
        {
            Thread.Sleep(500);
        }
        return allObjectAttributes;
    }

I also tried Spawn Multiple Threads for work then wait until all finished

I still get a null in one of the arraylist items in allObjectAttributes.

However, when I do

public ArrayList GetAllObjectAttributes()
    {
        ArrayList allObjectAttributes = new ArrayList();
        allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Folder)));
        allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.XMLFile)));
        allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.TextFile)));
        allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Parent)));
        return allObjectAttributes;
    }

I never get a null item in the arraylist items.

  1. What am i doing wrong to wait until all threads complete?
  2. Any other advise, so that the arraylist is returned only after all the 4 threads complete execution.
private List GetObjectAttributes(TreeViewAttrs tv)
{
    List objectAttributes = new List();
    string command = "COMMAND_TO_EXECUTE";
    if (command != "")
    {
        List results = RunCommand(command);
        if (results == null) { return null; }
        if (results.Count > 0)
        {
            foreach (string result in results)
            {
                if (!result.Contains("" + tv + ""))
                {
                    string[] res = reformatResponseString(result); //reformat the strings as per custom structure
                    if (res != null) { objectAttributes.Add(res); }
                }
            }
            return objectAttributes;
        }
    }
    return null;
}
Community
  • 1
  • 1
  • 2
    It's a pretty bad idea to mutate a shared, non-thread-safe instance (`ArrayList` in your case) from multiple threads. Just use a `ConcurrentBag` or, if you're on .NET 4.5, use `Task.WhenAll`, which actually produces an array of task results. – Kirill Shlenskiy Dec 09 '15 at 06:13
  • Can any one help to format the last paragraph of my question? I tried formatting it with 4 white spaces (as i did in the morning), but its not working. Seems like i'm missing so many things today. LOL – Vinay Sathyanarayana Dec 09 '15 at 14:58
  • 1
    You *definitely* have a problem with null propagation somewhere. Most likely a race or maybe a `ThreadStatic` or `ThreadLocal` variable hiding deeper down the call tree. I am not seeing anything wrong with the posted definition of `GetObjectAttributes` though - the function seems pure, so I'd be looking at `RunCommand` and `reformatResponseString` - that's an exercise for you though. Just create some conditional breakpoints on `if (something == null)` along your execution chain and run under debugger - sooner or later you'll catch your culprit. – Kirill Shlenskiy Dec 09 '15 at 15:05
  • But the same code never, never returns a null even when executed a hundred time, without using threads. This is killing me. :) – Vinay Sathyanarayana Dec 09 '15 at 15:11

2 Answers2

8

Slightly improved by using a thread-safe collection (.NET 4.0 compatible):

List<Task> taskList = new List<Task>();
ConcurrentBag<object> allObjectAttributes = new ConcurrentBag<object>();

taskList.Add(Task.Factory.StartNew(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Folder))));
taskList.Add(Task.Factory.StartNew(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.XMLFile))));
taskList.Add(Task.Factory.StartNew(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.TextFile))));
taskList.Add(Task.Factory.StartNew(() => allObjectAttributes.Add(GetObjectAttributes(TreeViewAttrs.Parent))));

Task.WaitAll(taskList.ToArray());

return allObjectAttributes;

Alternative approach: use Task.Result once all tasks have completed (thread-safe collection no longer required as only one thread modifies ArrayList):

Task<object>[] taskList = {
    Task.Factory.StartNew(() => (object)GetObjectAttributes(TreeViewAttrs.Folder)),
    Task.Factory.StartNew(() => (object)GetObjectAttributes(TreeViewAttrs.XMLFile)),
    Task.Factory.StartNew(() => (object)GetObjectAttributes(TreeViewAttrs.TextFile)),
    Task.Factory.StartNew(() => (object)GetObjectAttributes(TreeViewAttrs.Parent))
};

Task.WaitAll(taskList);

ArrayList allObjectAttributes = new ArrayList();

foreach (Task<object> task in taskList) {
    allObjectAttributes.Add(task.Result);
}

return allObjectAttributes;

Significantly improved by using Task.WhenAll (.NET 4.5 only):

object[] allObjectAttributes = await Task.WhenAll(
    Task.Run(() => GetObjectAttributes(TreeViewAttrs.Folder)),
    Task.Run(() => GetObjectAttributes(TreeViewAttrs.XMLFile)),
    Task.Run(() => GetObjectAttributes(TreeViewAttrs.TextFile)),
    Task.Run(() => GetObjectAttributes(TreeViewAttrs.Parent))
);

return allObjectAttributes;

*Note: I used object as the generic parameter as you left the return type of GetObjectAttributes unspecified.

Kirill Shlenskiy
  • 9,367
  • 27
  • 39
  • 2
    I agree `Task.WhenAll` is the best choice here. – Cheng Chen Dec 09 '15 at 06:19
  • I tried using ConcurrentBag, and still I get nulls in the response. The methods return List. I tried a lot of options, still, some of the results are null in the returned bag. But if i do it without threads, none of them are returned null. Not sure where i'm missing. I'll continuously try today and tomorrow and update this thread. I'm not gonna leave this till i get the expected results. Thanks all for your valuable time and feed back. – Vinay Sathyanarayana Dec 09 '15 at 14:41
  • @VinaySathyanarayana, in that case I blame `GetObjectAttributes` - there must be a race in it which prevents it from producing expected results when invoked in parallel. – Kirill Shlenskiy Dec 09 '15 at 14:44
0

Task.WaitAll() that you used in your first example should work as intended. However, I suspect that the issue you're having has to do more with thread safety of the collection you use, ArrayList, rather than the fact that the completion of all task is not waited upon. ArrayList offers no thread safety, so you should look at other ways, either through use of a locking mechanism or through the use of a thread-safe collection, such as ConcurrentBag (https://msdn.microsoft.com/en-us/library/dd381779(v=vs.110).aspx).

B.K.
  • 9,982
  • 10
  • 73
  • 105