Pinging all computers in network with TPL

Question

I'm developing an application that manages devices in the network, at a certain point in the applicaiton, I must ping (actually it's not a ping, it's a SNMP get) all computers in the network to check if it's type is of my managed device.

My problem is that pinging all computers in the network is very slow (specially because most of them won't respond to my message and will simply timeout) and has to be done asynchronously.

I tried to use TLP to do this with the following code:

public static void FindDevices(Action<IPAddress> callback)
{
    //Returns a list of all host names with a net view command
    List<string> hosts = FindHosts();

    foreach (string host in hosts)
    {
        Task.Run(() =>
        {
            CheckDevice(host, callback);
        });
    }
}

But it runs VERY slow, and when I paused execution I checked threads window and saw that it only had one thread pinging the network and was thus, running tasks synchronously.

When I use normal threads it runs a lot faster, but Tasks were supposed to be better, I'd like to know why aren't my Tasks optimizing parallelism.

**EDIT** Comments asked for code on CheckDevice, so here it goes:

    private static void CheckDevice(string host, Action<IPAddress> callback)
    {
        int commlength, miblength, datatype, datalength, datastart;
        string output;
        SNMP conn = new SNMP();


        IPHostEntry ihe;
        try
        {
            ihe = Dns.Resolve(host);
        }
        catch (Exception)
        {
            return;
        }
        // Send sysLocation SNMP request
        byte[] response = conn.get("get", ihe.AddressList[0], "MyDevice", "1.3.6.1.2.1.1.6.0");

        if (response[0] != 0xff)
        {
            // If response, get the community name and MIB lengths
            commlength = Convert.ToInt16(response[6]);
            miblength = Convert.ToInt16(response[23 + commlength]);

            // Extract the MIB data from the SNMP response
            datatype = Convert.ToInt16(response[24 + commlength + miblength]);
            datalength = Convert.ToInt16(response[25 + commlength + miblength]);
            datastart = 26 + commlength + miblength;
            output = Encoding.ASCII.GetString(response, datastart, datalength);
            if (output.StartsWith("MyDevice"))
            {
                callback(ihe.AddressList[0]);
            }
        }
    }

Also a List is not thread safe and shouldn't be used. [You should use a ConcurrentBag](http://stackoverflow.com/questions/5874317/thread-safe-listt-property). This could be the root of your problem. Does CheckDevice have a lock in it by any chance? — Liam, Jul 01 '14 at 12:14
It is quite curious that there is only one thread. Post the code of CheckDevice. — usr, Jul 01 '14 at 12:22
So far I couldn't spot the problem. Let's try something. Replace the dns check with `Task.Run(() => Thread.Sleep(10000))` and see if you now see many threads running. You should, assuming that `hosts` really contains many work items (how many exactly?). — usr, Jul 01 '14 at 12:30
Hosts have 35 items in my network right now, but depends on the time of the day — Eduardo Wada, Jul 01 '14 at 12:33
Indeed, by replacing CheckDevice(host, callback); for a sleep I can see many threads sleeping — Eduardo Wada, Jul 01 '14 at 12:38
Liam, Replacing the list for a Concurrent Bag worked, make that an answer and I'll accept it — Eduardo Wada, Jul 01 '14 at 12:43
possible duplicate of [Starting Tasks In foreach Loop Uses Value of Last Item](http://stackoverflow.com/questions/4684320/starting-tasks-in-foreach-loop-uses-value-of-last-item) — Liam, Jul 02 '14 at 07:41

score 0 · Accepted Answer · edited May 23 '17 at 11:49

0

Your issue is that you are iterating a none thread safe item the List.

If you replace it with a thread safe object like the ConcurrentBag you should find the threads will run in parallel.

I was a bit confused as to why this was only running one thread, I believe it is this line of code:

try
{
    ihe = Dns.Resolve(host);
}
catch (Exception)
{
    return;
}

I think this is throwing exceptions and returning; hence you only see one thread. This also ties into your observation that if you added a sleep it worked correctly.

Remember that when you pass a string your passing the reference to the string in memory, not the value. Anyway, the ConcurrentBag seems to resolve your issue. This answer might also be relevant

edited May 23 '17 at 11:49

Community

1
1

answered Jul 01 '14 at 13:09

Liam

27,717
28
128
190

Dns.Resolve(host); throws an exception after a timeout, so I still see the threads stopped on that line after replacing it for a ConcurrentBag, from the tests I'd say TPL was somehow detecting that I was acessing a member from a non-thread safe structure and serializing the tasks to ensure predictable results. – Eduardo Wada Jul 01 '14 at 15:49
Also, Dns.Resolve(host); was not the line I replaced with the sleep, it was CheckDevice(host, callback); which in turn, made the host variable became unused – Eduardo Wada Jul 01 '14 at 16:00
But CheckDevice calls Resolve? Also resolve is the only point you use the string host. It's all the same issue. The reference type is moving on outside of the Task. This is either making the Resolve fail or updating the List in an unsafe manner. What was the type of exception being thrown? – Liam Jul 01 '14 at 16:04
Indeed, CheckDevice calls Resolve, and the Exception is a SocketException with a message saying the requested name is not valid, but when I replace ihe = Dns.Resolve(host); for ihe = Dns.Resolve("invalid host"); I can still see parallel resolves for invalid hosts, the problem seems to be just accessing the host variable, even if it's not updated – Eduardo Wada Jul 01 '14 at 16:28
So the DNS.ResolveHost isn't liking the reference type updating by the looks of it. I would expect the SocketException to have an (or a few) InnerException(s)? Don't forget the string will update constantly because it is within a foreach. I think the ConsurrentBag is actually breaking the reference(s) between the value returned from the foreach and the value stored within collection. Which is why changing list type fixes the issue. – Liam Jul 01 '14 at 16:34
It doesn't have an inner exception, and I also noticed that when running the real code there are more messages on exceptions being thrown: "the requested name is valid but does not have an ip address", still I'm suspecting that host variable was getting locked somewhere, since the code wasn't giving unexpected results and was listing devices correctly – Eduardo Wada Jul 01 '14 at 16:44

Pinging all computers in network with TPL

1 Answers1