C# Windows Async Pinging Network - different results each run

Question

I've written a class that asynchronously pings a subnet. It works, however, the number of hosts returned will sometimes change between runs. Some questions:

• Am I doing something wrong in the code below?
• What can I do to make it work better?

The ScanIPAddressesAsync() method is called like this:

NetworkDiscovery nd = new NetworkDiscovery("192.168.50.");
nd.RaiseIPScanCompleteEvent += HandleScanComplete;
nd.ScanIPAddressesAsync();

namespace BPSTestTool
{

    public class IPScanCompleteEvent : EventArgs
    {
        public List<String> IPList { get; set; }

        public IPScanCompleteEvent(List<String> _list)
        {
            IPList = _list;
        }
    }

    public class NetworkDiscovery
    {
        private static object m_lockObj = new object();
        private List<String> m_ipsFound = new List<string>();
        private String m_ipBase = null;


        public List<String> IPList
        {
            get { return m_ipsFound; }
        }

        public EventHandler<IPScanCompleteEvent> RaiseIPScanCompleteEvent;

        public NetworkDiscovery(string ipBase)
        {
            this.m_ipBase = ipBase;
        }

        public async void ScanIPAddressesAsync()
        {
            var tasks = new List<Task>();

            m_ipsFound.Clear();
            await Task.Run(() => AsyncScan());
            return;
        }

        private async void AsyncScan()
        {
            List<Task> tasks = new List<Task>();
            for (int i = 2; i < 255; i++)
            {
                String ip = m_ipBase + i.ToString();

                if (m_ipsFound.Contains(ip) == false)
                {
                    for (int x = 0; x < 2; x++)
                    {
                        Ping p = new Ping();
                        var task = HandlePingReplyAsync(p, ip);
                        tasks.Add(task);
                    }
                }
            }
            await Task.WhenAll(tasks).ContinueWith(t =>
            {
                OnRaiseIPScanCompleteEvent(new IPScanCompleteEvent(m_ipsFound));
            });
        }

        protected virtual void OnRaiseIPScanCompleteEvent(IPScanCompleteEvent args)
        {
            RaiseIPScanCompleteEvent?.Invoke(this, args);
        }

        private async Task HandlePingReplyAsync(Ping ping, String ip)
        {
            PingReply reply = await ping.SendPingAsync(ip, 1500);

            if ( reply != null && reply.Status == System.Net.NetworkInformation.IPStatus.Success)
            {
                lock (m_lockObj)
                {
                    if (m_ipsFound.Contains(ip) == false)
                    {
                        m_ipsFound.Add(ip);
                    }
                }
            }
        }
    }
}

Answer 1

One problem I see is async void. The only reason async void is even allowed is only for event handlers. If it's not an event handler, it's a red flag.

Asynchronous methods always start running synchronously until the first await that acts on an incomplete Task. In your code, that is at await Task.WhenAll(tasks). At that point, AsyncScan returns - before all the tasks have completed. Usually, it would return a Task that will let you know when it's done, but since the method signature is void, it cannot.

So now look at this:

await Task.Run(() => AsyncScan());

When AsyncScan() returns, then the Task returned from Task.Run completes and your code moves on, before all of the pings have finished.

So when you report your results, the number of results will be random, depending on how many happened to finish before you displayed the results.

If you want make sure that all of the pings are done before continuing, then change AsyncScan() to return a Task:

private async Task AsyncScan()

And change the Task.Run to await it:

await Task.Run(async () => await AsyncScan());

However, you could also just get rid of the Task.Run and just have this:

await AsyncScan();

Task.Run runs the code in a separate thread. The only reason to do that is in a UI app where you want to move CPU-heavy computations off of the UI thread. When you're just doing network requests like this, that's not necessary.

On top of that, you're also using async void here:

public async void ScanIPAddressesAsync()

Which means that wherever you call ScanIPAddressesAsync() is unable to wait until everything is done. Change that to async Task and await it too.

Answer 2

This code needs a lot of refactoring and bugs like this in concurrency are hard to pinpoint. My bet is on await Task.Run(() => AsyncScan()); which is wrong because AsyncScan() is async and Task.Run(...) will return before it is complete.

My second guess is m_ipsFound which is called a shared state. This means there might be many threads simultaneously reading and writing on this. List<T> is not a data type for this.

Also as a side point having a return in the last line of a method is not adding to the readability and async void is a prohibited practice. Always use async Task even if you return nothing. You can read more on this very good answer.