4
\$\begingroup\$

I have a class which is responsible for checking whether a website is up or down. I've made it to run asynchronously, but I would like critique if this is a good/bad way or if there is another better way to do it.

class UrlChecker {
    private readonly IValidationCondition _condition;
    private readonly Dictionary<string, UrlStatus> _status = new Dictionary<string, UrlStatus>();
    private readonly object _lock = new object();

    public UrlChecker(IValidationCondition condition) {
        _condition = condition;
    }

    public void CheckRange(IEnumerable<string> urls, Action<Dictionary<string, UrlStatus>> callback) {
        var options = new ParallelOptions {MaxDegreeOfParallelism = 5};
        Parallel.ForEach(urls, options, Check);

        callback(_status);
    }

    private void Check(string url) {
        Console.WriteLine("Checking " + url);

        var req = (HttpWebRequest) WebRequest.Create(url);
        req.Timeout = 10 * 10000; // 10 seconds

        HttpWebResponse resp;

        try {
            resp = (HttpWebResponse)req.GetResponse();    
        }
        catch(WebException ex) {
            // We got an exception, consider it as down
            lock (_lock)
                _status.Add(url, UrlStatus.Down);
            return;
        }

        if(resp.StatusCode != HttpStatusCode.OK) {
            lock (_lock)
                _status.Add(url, UrlStatus.Down);
            return;
        }

        using(var reader = new StreamReader(resp.GetResponseStream())) {
            // Check for empty response
            var html = reader.ReadToEnd();
            if(string.IsNullOrEmpty(html)) {
                lock(_lock) {
                    _status.Add(url, UrlStatus.Down);
                }
            }

            // Validate against condition
            if(!_condition.IsValid(html)) {
                lock(_lock) {
                    _status.Add(url, UrlStatus.Down);
                }
                return;
            }
        }

        // We reached the end without problems, it's a valid url
        lock(_lock) {
            _status.Add(url, UrlStatus.OK);
        }
    }
}

It's called like so:

checker.CheckRange(urls, status => {
    if(status.Any(x => x.Value == UrlStatus.Down))
        EmailFailing(message);
});

The second parameter is obviously a callback that's invoked when all checks are done.

Am I locking correctly?

Is this an acceptable way of doing it? Checking with Fiddler proves that it's working correctly, but is there a better way?

\$\endgroup\$
  • \$\begingroup\$ @Kiquenet Unfortunately not at the moment. I'll see if I can extract it from my project. \$\endgroup\$ – alexn Aug 22 '12 at 11:44
  • \$\begingroup\$ It would be great, thx \$\endgroup\$ – Kiquenet Aug 22 '12 at 19:56
4
\$\begingroup\$

I don't see how this runs asynchronously. callback(_status); will still only be called after all processing is finished, parallel or not. In this case this makes the callback rather redundant, as it does the same as a simple return value.

If you want to make it asynchronous you'll need to store the callback as a member variable, but return immediately in CheckRange. Start the actual processing on a separate thread by e.g. using a BackgroundWorker. Once this execution completes, you can call the stored callback with the result.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is not all checks run asynchronously (5 at a time)? The callback is invoked when all urls are checked, and that's what i want. \$\endgroup\$ – alexn Apr 15 '11 at 13:15
  • \$\begingroup\$ That's called in parallel, hence the name of Parallel.ForEach. Asynchronous means: Calling functions asynchronously causes the system to execute them in the background on a secondary thread while the calling function continues to do other work. If that's not the behavior you want, you can just return _status; instead of the callback. \$\endgroup\$ – Steven Jeuris Apr 15 '11 at 13:19
  • \$\begingroup\$ ah, of course. I mean parallel. English is not my primary language and I thought async and parallel had the same meaning. Am I approaching parallelism in a correct way? \$\endgroup\$ – alexn Apr 15 '11 at 14:25
  • \$\begingroup\$ @alexn: I don't see any problems no. I would benchmark this however and see whether the speed of execution is indeed sped up. Otherwise there is no point to this approach. And again, just remove the Action and use a return value. \$\endgroup\$ – Steven Jeuris Apr 15 '11 at 14:43
3
\$\begingroup\$

The only issue I can see is if your implementation of IValidationCondition is not thread safe. It doesn't sound like something that would have any state, but I figured it should probably be stated explicitly in the code's documentation (either as a requirement of the interface or the UrlChecker's constructor).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That's a good point that i haven't thought of. But you're correct, it does not have any state. Thanks. \$\endgroup\$ – alexn Apr 16 '11 at 8:40
3
\$\begingroup\$

the amount you are locking is not so cool..... I'd make an "Add" method that does your locking for you

general rule of thumb with multithreading..... do all your synchronization in the most minimal tiniest piece of code as possible. ie, try and make it all go through one spot so you have limited points of failure. ( personally I tend to put things through synchronized queues )

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Have you considered using a ConcurrentDictionary rather than implementing the locking yourself? It's bundled with the Reactive Extensions if you're still on .NET 3.5.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, that's a good tip. I was not aware of ConcurrentDictionary but it seems to be exactly what i want. \$\endgroup\$ – alexn Apr 19 '11 at 6:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.