C# Ping - Checking availability of hosts

Question

Simple Console Application for checking if one of the hosts is available. The implementation should be for around 5 hosts to check with different response times and the method should return fast when one Host can be pinged. Is the asynchronous implementation O.K.?

using System;
using System.Net.NetworkInformation;
using System.Threading;
using System.Threading.Tasks;

namespace PingMe
{
    class Program
    {
        private static volatile bool _result;

        private readonly CancellationTokenSource _source;

        private readonly ParallelOptions _parallelOptions;
        private readonly string[] _hostsToPing = {"google.com", "reddit.com", "netflix.com"};

        private Program()
        {
            _source = new CancellationTokenSource();
            var token = _source.Token;
            _parallelOptions = new ParallelOptions
            {
                CancellationToken = token
            };
        }

        private bool IsOneOfTheHostsAvailable()
        {
            try
            {
                Parallel.ForEach(_hostsToPing, _parallelOptions, (host) =>
                {
                    try
                    {
                        var ping = new Ping();
                        var reply = ping.Send(host);
                        _parallelOptions.CancellationToken.ThrowIfCancellationRequested();
                        if (reply?.Status == IPStatus.Success)
                        {
                            _result = true;
                            Console.WriteLine($"Pinged {host}");
                            if (!_source.IsCancellationRequested)
                                _source.Cancel();
                        }
                    }
                    catch (Exception)
                    {
                        // we are only interested if one of the pings is successful, we don't care about the reason
                    }
                });
            }
            catch (OperationCanceledException)
            {
                // we are done
            }

            return _result;
        }


        private static void Main()
        {
            var program = new Program();
            var oneOfTheHostsIsAvailable = program.IsOneOfTheHostsAvailable();
            Console.WriteLine($"One of the hosts is available: {oneOfTheHostsIsAvailable}");
        }
    }
}

Answer 1

Except all that @JAD said, which I totally agree with. I will give me five cents also after taking your comment in mind.

I want to cancel the other pings when one of the ping responses is successful. This should be the default behaviour - so I don't want to expose this implementation detail to the other clients.

First and foremost, there is a much cleaner and concise way of canceling your Parallel.ForEach. Check ParallelLoopState.Break method, I believe it does exactly the same thing you want to achieve without polluting your class with any unnecessary to business logic stuff - like CancellationTokenSource or ParallelOptions.

Second, you probably forgot to put Ping class in using statement. Ping class inherits IDisposable type which means that it will be a good practice to put it in using statement.

Third, I don't mind having fancy stuff like volatile field, but if I want to use something that fancy I want my code to "scream out loud" - "Hey, this makes sense here and it's probably the only way of achieving it." However, this is not the case here. It is not obvious to me why this property is static in the first place (perhaps the example is not good enough) also I don't see why it is volatile as well.

Although the volatile keyword can help you in thread safety in certain situations, it is not a solution to all of your thread concurrency issues. You should know that marking a variable or an object as volatile does not mean you don’t need to use the lock keyword. The volatile keyword is not a substitute for the lock keyword. It is only there to help you avoid data conflicts when you have multiple threads trying to access the same data.¹

You are never accessing _result in your multithreaded code (unless I am wrong), you only try to assign it. With the same success, you can remove volatile keyword and the code will work as intended (again, unless I am missing something) because you are not changing the state. In other words, you don't care if _result is set to true one time, two times or three times and in what order simply because this is your business logic - you only need to know if at least one host is available.

With all the comments above the code looks like this

using System;
using System.Net.NetworkInformation;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    class Program
    {
        private bool _result = false;
        private readonly string[] _hostsToPing = { "netflix.com", "google.com", "reddit.com" };

        private bool IsOneOfTheHostsAvailable()
        {
            Parallel.ForEach(_hostsToPing, (host, state) =>
            {
                if (state.ShouldExitCurrentIteration) return; // <-- personal preference
                // I just hate .NET world putting brackets and new lines for nothing

                using (var ping = new Ping())
                {
                    var reply = ping.Send(host);
                    if (reply?.Status == IPStatus.Success)
                    {
                        _result = true;
                        Console.WriteLine($"Pinged {host}");
                        state.Break();
                    }
                }

            });

            return _result; // you can even further remove _result by simply returning
            // IsCompleted property of ParallelLoopResult but many will argue this is more 
            // readable so it's up to you
        }


        private static void Main()
        {
            var program = new Program();
            var oneOfTheHostsIsAvailable = program.IsOneOfTheHostsAvailable();
            Console.WriteLine($"One of the hosts is available: {oneOfTheHostsIsAvailable}");
        }
    }
}

In the end, I will try to emphasize one thing - Don't use features you don't fully understand. This might not be the case for you but maybe the case for others, it is better that you do something in 50 lines of code but fully understands it than coding it in 5-6 lines with fancy stuff but then don't know why it works. Time will pass, you will gain more experience and you will eventually come to the place where you will do it in 5-6 lines, but don't try to force this process.

Answer 2

It doesn't make sense to have the CancellationTokenSource private like you have now. Who is going to be able to cancel the operation? The user of your class can't access the source or provide their own token. The class doesn't offer any API to cancel.

Instead, you should alter the private bool IsOneOfTheHostsAvailable() method to accept a CancellationToken as input. You can then let the user care about creating a source and possibly cancelling the operation. Or, if you don't feel like offering the option to the user, you should just do away with the cancellation token as a whole. Right now it isn't doing anything.