Ayende @ Rahien

Refunds available at head office

Reviewing NAppUpdate

I was asked to review NAppUpdate, a simple framework for providing auto-update support to .NET applications, available here. Just for to make things more interesting, the project leader of NAppUpdate is Itamar, who works for Hibernating Rhinos, and we actually use NAppUpdate in the profiler.

I treated it as an implementation detail and never actually looked at it closely before, so this is the first time that I am actually going over the code. On first impression, there is nothing that makes me want to hurl myself to the ocean from a tall cliff:

image

Let us dig deeper, and almost on the first try, we hit something that I seriously dislike.

image

Which leads to this:

public static class Errors
{
    public const string UserAborted = "User aborted";
    public const string NoUpdatesFound = "No updates found";
}

And I wouldn’t mind that, except that those are used like this:

image

There are actually quite a lot of issues with this small code sample. To start with, LastestError? Seriously?!

LatestError evoke strong memories of GetLastError() and all the associated fun with that.

It doesn’t give you the ability to multiple errors, and it is a string, so you can’t put an exception into it (more on that later).

Also, note how this work with the callback and the return code. Both of which have a boolean for success/failure. That is wrong.

That sort of style was valid for C, but .NET, we actually have exceptions, and they are actually quite a nice way to handle things.

Worse than that, it means that you have to check the return value, then go the LatestError and check what is going on there, except… what happen if there was an actual error?

image

Note the todo, it is absolutely correct. You really can’t just call a ToString() on an exception and get away with it (although I think that you should). There are a lot of exceptions where you would simply won’t get the required information. ReflectionException, for example, ask you to look at the LoaderExceptions property, and there are other such things.

In the same error handling category, this bugs me:

image

This is an exception that implements the serialization exception, but doesn’t have the [Serializable] attribute, which all exceptions should have.

Moving on, a large part of what NAU does is to check remote source for updates, then apply it. I liked this piece of code:

public class AppcastReader : IUpdateFeedReader
{
    // http://learn.adobe.com/wiki/display/ADCdocs/Appcasting+RSS

    #region IUpdateFeedReader Members

    public IList<IUpdateTask> Read(string feed)
    {
        XmlDocument doc = new XmlDocument();
        doc.LoadXml(feed);
        XmlNodeList nl = doc.SelectNodes("/rss/channel/item");

        List<IUpdateTask> ret = new List<IUpdateTask>();

        foreach (XmlNode n in nl)
        {
            FileUpdateTask task = new FileUpdateTask();
            task.Description = n["description"].InnerText;
            //task.UpdateTo = n["enclosure"].Attributes["url"].Value;
            task.UpdateTo = n["enclosure"].Attributes["url"].Value;

            FileVersionCondition cnd = new FileVersionCondition();
            cnd.Version = n["appcast:version"].InnerText;
            task.UpdateConditions.AddCondition(cnd, BooleanCondition.ConditionType.AND);

            ret.Add(task);
        }

        return ret;
    }

    #endregion
}

This is integrating with an external resources, and I like that this is simple to read and understand. I don’t have a lot of infrastructure going on in here that I have to deal with just to get what I want done.

There is a more complex feed reader for an internal format that allows you to use the full option set of NAU, but it is a bit complex (it does a lot more, to be fair), and again, I dislike the error handling there.

Another thing that bugged me on some level was this code:

image

The issue, and I admit that this is probably academic, is what happen if the string is large. I try to avoid exposing API that might force users to materialize a large data set in memory. This has implications on the working set, the large object heap, etc.

Instead, I would probably exposed a TextReader or even a Stream.

Coming back to the error handling, we have this:

FileDownloader fd = null;
if (Uri.IsWellFormedUriString(url, UriKind.Absolute))
    fd = new FileDownloader(url);
else if (Uri.IsWellFormedUriString(baseUrl, UriKind.Absolute))
    fd = new FileDownloader(new Uri(new Uri(baseUrl, UriKind.Absolute), url));
else if (Uri.IsWellFormedUriString(new Uri(new Uri(baseUrl), url).AbsoluteUri, UriKind.Absolute))
    fd = new FileDownloader(new Uri(new Uri(baseUrl), url));

if (fd == null)
    throw new ArgumentException("The requested URI does not look valid: " + url, "url");

My gripe is with the last line. Instead of doing it like this, I would have created a new Uri and let it throw the error. It is likely that it will have much more accurate error information about the actual reason this Uri isn’t valid.

On the gripping hand, we have this guy:

image

This is the abstraction that NAU manages, the Prepare() method is called to do all the actual work (downloading files, for example) and Execute() is called when we are done and just want to do the actual update.

I know that I am harping about this, but this is really important. Take a look at another error handling issue (this is representative of the style of coding inside this particular class) in the RegistryTask:

image

So, I have an update that is failing at a customer site. What error do I have? How do I know what went wrong?

Worse, here is another snippet from the FileUpdateTask:

image

For some things, an exception is thrown, for other, we just return false.

I can’t follow the logic for this, and trying to diagnose issues in production can be… challenging.

In summary, I went through most of the actual important bits of NAppUpdate, and like in most reviews, I focused on the stuff that can be improved. I didn’t touch on the stuff it does well, and that is its job.

We have been using NAppUpdate for the last couple of years, with very few issues. It quite nicely resolves the entire issue of auto updates to the point were in our code, we only need to call a few methods and it takes cares of the entire process for us.

Major recommendations from this codebase:

  • Standardize the error handling approach. It is important.
  • And even more important, logging is crucial to be able to diagnose issues in the field. NAU should log pretty much everything it does and why it does it.

This will allow later diagnosal of issues with relative ease, vs. “I need you to reproduce this and then break into the debugger”.

Comments

Itamar Syn-Hershko
07/04/2012 11:01 PM by
Itamar Syn-Hershko

Ack

The highest priority has been to provide a super-flexible, and working, update library for .NET. I guess now that we have actually come there, is the time to make it super-robust as well.

UpdateManager is a singleton, and maintains a strict state, so it isn't really meaningless to hold more than one error at a given time. That at least was the original thought behind the basic error handling of the C-style LatestError. And yes, it smells.

You raise good points with regards to logging and being able to quickly understand what's going on in case of a failure. So, error handling is going to be standardized, full logging is going to be implemented, and as a bonus better async support is going to replace the callback-based one.

First updates for making NAU air-tight have already been committed to the github repo.

Thanks for prompting this important update.

James McKay
07/05/2012 12:28 PM by
James McKay

Poor error handling code is an extremely common problem, and I think that a large part of the problem is that there is very little decent guidance anywhere about how to use structured exception handling correctly.

For example, far too many tutorials and "best practice" articles tell you "only throw exceptions in cases that are exceptional" -- advice that IMO is worse than useless. It's like saying "Only eat foods that are edible," and that doesn't tell you for instance that strawberries are fine whereas deadly nightshade can kill you. Consequently I think that there's a lot of confusion among developers about when you should throw exceptions, when you should catch exceptions, and how you should deal with error conditions in general.

A far better guideline is that an exception means your method can't do what its name says that it does. Conversely, any kind of return value (without an exception) means that your method has successfully completed what its name says that it does.

Another important principle is "fail fast" -- a lot of developers seem to believe that by swallowing exceptions they're making their code robust. This all too often results in data being corrupted or incorrect results being displayed. That isn't making your code robust: it's making it dishonest.

tobi
07/05/2012 12:32 PM by
tobi

I like the notion that exceptions are just a special return value, one that cannot be ignored. A big benefit over return false (which I am seeing ALL over the codebase I am forced to work on).

Luiz Adilson Bon
07/05/2012 12:47 PM by
Luiz Adilson Bon

What is the advantages of NAppUpdate over the ClickOnce? And is there a new project called Shimmer that's promise to work like ClickOnce. Looking at the documentation of the projectsm looks like NAppUpdate is more complex to implement and only works after the application is installed.

olcay seker
07/05/2012 12:53 PM by
olcay seker

@Ayende "And even more important, logging is crucial to be able to diagnose issues in the field. NAU should log pretty much everything it does and why it does it." Any OS project link that shows mentioned approach would be awsome.

Ayende Rahien
07/05/2012 01:47 PM by
Ayende Rahien

Olcay, RavenDB and Rhino Service Bus follow that philosophy

Diogo Mafra
07/05/2012 01:55 PM by
Diogo Mafra

It seems that NAppUpdate offers many customizations in the update process. I haven't used it, but I think if you need those options, it will be a great choice.

In my last project I needed a very simple update processes, similar to the Google Chrome update process. It just downloads the delta of the changed files and the next time the application is executed, the updated application is used.

I made this auto-update library open source a few months ago. The library is functional, but I think it could be refactored/cleaned.

Here is the source: https://github.com/diogomafra/AppUpdater

And here is the doc: http://diogomafra.github.com/AppUpdater/

Arcan
07/05/2012 02:39 PM by
Arcan

Hi ayende, concerning error handling in general i thought it is an infrastructure concern, can you give an idea (smart way) to handle it ? Thank you in advance

Daniel Lang
07/05/2012 06:05 PM by
Daniel Lang

Arcan, why don't you just look at the RavenDB source code. Since it is his 'baby', there is a good chance that he did it the way he wanted it to be...

David
07/11/2012 06:28 AM by
David

The exception handling exampled is indeed horrible.

However, for the sake of accuracy... By default, an Exception's ToString is designed to show all pertinent information, including the ToStrings of the InnerExceptions. So long as a given derivation of Exception ALSO correctly implements ToString to display any additional information required above and beyond the inner exception, then ex.ToString works great. In other words: If any derivation of Exception fails to print all the details you should need to diagnose the problem with ToString, IT IS A BUG of that derivation.

Ayende Rahien
07/11/2012 08:17 AM by
Ayende Rahien

David, I agree on principle. Unfortunately common exceptions such as ReflectionException does NOT respect that.

Comments have been closed on this topic.