Should TryFoo ever throw an exception?

Question

A common pattern in the .NET Framework is the TryXXX pattern (I don't know if that's what they really call it), in which the called method attempts to do something, returning True if it was successful, or False if the operation failed. A good example is the generic Dictionary.TryGetValue method.

Documentation for these methods say that they will not throw exceptions: that failure will be reported in the method return value. All good so far.

I recently encountered two different circumstances in which .NET Framework methods that implement the TryXXX pattern threw exceptions. See http://stackoverflow.com/questions/1148278/bug-in-system-random-constructor and http://stackoverflow.com/questions/1120283/uri-trycreate-throws-uriformatexception for details.

In both cases, the TryXXX method called other methods that threw unexpected exceptions, and those exceptions escaped. My question: does this break the implied contract of not throwing exceptions?

Put another way, if you were writing TryFoo, would you guarantee that exceptions cannot escape, by writing it like this?

public bool TryFoo(string param, out FooThing foo)
{
    try
    {
        // do whatever
        return true;
    }
    catch
    {
        foo = null;
        return false;
    }
}

It's tempting, because that guarantees no exceptions will escape, thereby honoring the implied contract. But it's a bug hider.

My gut feeling, based on my experience, says that this is a really bad idea. But if TryFoo lets some exceptions escape, then what it's really saying is, "I won't throw any exceptions that I know how to handle," and then the whole idea of the contract "I won't throw exceptions" is thrown out the window.

So, what's your opinion? Should TryFoo handle all exceptions, or just those that it's expecting to happen? What's your reasoning?

Answer 1

The idea behind TryXXX is that it will not throw a specific type of exception that will normally occur if you simply called the corresponding XXX call - the consumer of the method knows that they are ignoring any exception that would normally occur.

It's tempting, because that guarantees no exceptions will escape, thereby honoring the implied contract.

This is not completely true - there are a few exceptions that can escape a try/catch (e.g. OutOfMemoryException, StackOverflowException, etc.).

Answer 2

You almost should never catch all exceptions. If you must implement TryXXX using a try/catch pair around XXX (rather than the preferred, XXX implemented in terms of TryXXX and throwing on false result), catch the specific expected exception(s) only.

It's OK for TryXXX to throw if there is a semantically different problem, such as a programmer error (perhaps passing null where it's not expected).

Answer 3

It depends. TryParseFoo should catch all parsing exceptions for stuff like null or misformatted strings. But there are also unrelated exceptions (ThreadAbortException, say) that should not be automatically quieted.

In Python (bear with me), you can cause a lot of trouble by catching KeyboardInterrupt exceptions, which basically means your program keeps running even if you Ctrl-C out of it.

My rule of thumb is that you should use TryFoo when you want to Foo without cleaning up your inputs. If TryFoo catches errors unrelated to Fooing your input, it's being too aggressive.

Answer 4

That depends on what the method is trying to do.

If the method for example reads a string value from a data reader and tries to parse it into an integer, it should not throw an exception if the parsing fails, but it should throw an exception if the data reader can not read from the database or if the value that you are reading is not a string at all.

The method should follow the general principle for catching exceptions that you should only catch what you know how to handle. If some completely different exception occurs, you should let that bubble up to some other code that knows how to handle it.

Answer 5

Put another way, if you were writing TryFoo, would you guarantee that exceptions cannot escape, by writing it like this?

No, I would not swallow exceptions in my TryFoo method. Foo depends on TryFoo, not the other way around. Since you mentioned the generic Dictionary, which has a GetValue and a TryGetValue method, I'd write my dictionary methods as follows:

public bool TryGetValue(T key, out U value)
{
    IList<KeyValue> kvCollection = internalArray[key.GetHashCode() % internalArray.Length];
    for(KeyValue kv in kvCollection)
    {
        if(kv.Key == key)
        {
            value = kv.Value;
            return true;
        }
    }
    value = default(U);
    return false;
}

public U GetValue(T key)
{
    U value;
    if (TryGetValue(key, out value))
    {
        return value;
    }
    throw new KeyNotFoundException(key);
}

So, GetValue depends on TryGetValue and throws an exception if TryGetValue returns false. This is much better than calling GetValue from TryGetValue and swallowing the resulting exception.

Answer 6

But if TryFoo lets some exceptions escape, then what it's really saying is, "I won't throw any exceptions that I know how to handle," and then the whole idea of the contract "I won't throw exceptions" is thrown out the window.

I think you said it yourself here :)

TryXXX should only guarentee that it wont throw exceptions it knows how to handle. If it doesn't know how to handle a given exception, why should it catch it? Same goes for you - If you don't know what to do, why would you catch it? BadAllocExceptions for example, well, (ussually) not much to do about those, except to catch them in your main try/catch block show some error message and try to shut the app down gracefully..

Answer 7

The two other StackOverflow questions you linked to seem to be genuine bugs. The one where Uri.TryCreate throws a UriFormatException for example is definitely a break in the contract, as the MSDN documentation for that method explicitly states:

Does not throw an exception if the Uri cannot be created.

The other one, TryDequeue, I can't find any documentation for, but it also seems to be a bug. If the methods were called TryXXX but the documentation explicitly stated that they could throw exceptions, then you may have a point.

Generally there will be two versions of this sort of method, one which throws exceptions and one which doesn't. For example, Int32.Parse and Int32.TryParse. The point of the TryXXX method isn't to hide exceptions. It's for situations where failing isn't actually an exceptional condition. For example, if you're reading in a number from the console and want to keep trying until a correct number is input, you don't want to keep catching exceptions. Similarly, if you want to get a value out of a dictionary but it is a perfectly normal case that it might not exist, you would use TryGetValue.

Summary: TryXXX should not throw exceptions and it's not designed to hide exceptions either. It's for cases where it is normal and non-exceptional to fail, and you would like to detect that case easily without the overhead and effort of catching an exception. Usually a non-TryXXX method, which does provide exceptions, is provided too, for cases when it is exceptional.

Answer 8

Catching and swallowing all exceptions as is done in your example is generally a bad idea. Some exceptions, most notably ThreadAbortException and OutOfMemoryException should not be swallowed. Actually, the first cannot be swallowed and will automatically be rethrown at the end of your catch block, but still.

Phil Haack has a blog entry about this exact topic.

Answer 9

I don't think that TryFoo must not throw exceptions. I think the contract of TryFoo is that it will handle invalid input as a normal case, not an exceptional one. That is, it will not throw an exception if it fails its task because of invalid input; if it fails for any other reason, it should throw an exception. My expectation after TryFoo returns is that either it handled my input, or my input was invalid. If neither of those things is true, I want an exception, so I know about it.

Answer 10

It's not a guarantee to not throw exceptions. Consider this trivial example on Dictionary ...

var dic = new Dictionary<string, string>() { { "Foo", "Bar" } };

string val = String.Empty;
string key = null;

dic.TryGetValue(key, out val); // oops

I sent a null key, I get a NullArgumentException. Reflector shows the code thusly ...

private int FindEntry(TKey key)
{
  if (key == null)
  {
    ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
   }
   // more stuff
}

In my mind the meaning of the contract is "If what you tried to do failed, I won't throw an exception", but that is far from "I will not throw any exceptions." Since you're probably not writing a framework, you could compromise with something like this ...

catch( Exception ex )
{
  Logger.Log( ex );
  Debug.Assert( false );
  foo = null;
  return false;
}

... and don't handle the TryXXX failure case with that catch block (so you don't have a bunch of trivial log entires). In that case, you will at least reveal the nature of the bug and identify it during dev. time as well as note it at runtime.

Answer 11

I once attended a great talk by someone who said he was involved in coming up with the exception handling strategy for the BCL. Unfortunately I've forgotten his name and I can't find my notes.

He described the strategy thus:

1. Method names must be verbs that describe the action taken by the method.
2. If the action described by the name does not take place for any reason, and exception must be thrown.
3. Where possible, a way of testing for and avoiding an upcoming exception should be provided. E.g. calling File.Open(filename) will thrown an exception if the file doesn't exist, but calling File.Exists(filename) first will let you avoid that (most of the time).
4. If there are demonstrable reasons (e.g. performance in a common case) an extra method can be added call TryXXX where XXX is the name of the original method. This method should be able to handle a single common failure mode and must return a boolean to indicate success or failure.

The interesting point here was #4. I clearly remember him stating the single failure mode part of the guidelines. Other failures should still throw exceptions.

Incidentally, he also said that the CLR team told him that the reason that .Net exceptions are slow is because they are implemented on top of SEH. They also said that there was no particular need for them to be implemented that way (apart from expediency), and if they ever got in to the top 10 of real performance problems for real customers, they'd think about re-implementing them to be faster!