Re-throwing the exception of a wrapped object

Question

I use ConcurrentDictionary<TKey, TValue> to implement a ConcurrentSet<T>.

public class ConcurrentSet<T> : ISet<T>
{
    private readonly ConcurrentDictionary<T, byte> collection;
}

ConcurrentDictionary<TKey, TValue> can't contain a pair with a null key.

// summary, param, returns...
/// <exception cref="ArgumentNullException">
///     <paramref name="item" /> is null.
/// </exception>
public bool Add(T item)
{
    // This throws an argument null exception if item is null.
    return this.collection.TryAdd(item, 0);
}

So, should I,

if (item == null)
    throw new ArgumentNullException("item", "Item must not be null.");

return this.collection.TryAdd(item, 0);

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException)
{
    throw new ArgumentNullException("item", "Item must not be null.");
}

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException x)
{
    // I know, but I don't want to preserve the stack trace
    // back to the underlying dictionary, anyway.
    throw x;
}

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException)
{
    // The thrown exception will have "key", instead of
    // "item" as the parameter's name, in this instance.
    throw;
}

What is the proper way to do this?

Check out this SO answer on re-throwing exceptions: http://stackoverflow.com/questions/881473/why-catch-and-rethrow-exception-in-c — George Johnston, Apr 17 '13 at 16:28
Could you please add information on what is the point in wrapping `ArgumentNullException` with `ArgumentNullException`? — Ilya Ivanov, Apr 17 '13 at 16:28
@IlyaIvanov: I think the idea is that since fundamental problem is that `ConcurrentSet.Add` was called with a null value, that's what the stack trace should show. Additionally, the thrown exception should indicate that the name of the null argument is "item" rather than "key". — supercat, Apr 17 '13 at 16:54
@IlyaIvanov: Like supercat says, the `ArgumentNullException` that is thrown by a `ConcurrentDictionary` have its `ParamName` property set to "key", instead of "item". That is not the expected behaviour for this class and it can confuse the consumers of `ConcurrentSet`, since it exposes an implementation detail both by the exception's `ParamName` property *and* by the stack trace that is originated from the `ConcurrentDictionary`. — Şafak Gür, Apr 18 '13 at 06:28
@wonko79: This is not a duplicate of the question you mentioned, since this is more about API design than error handling, actually. The consumers of ConcurrentSet should handle an ArgumentNullException if they pass a null value to the Add(T) method but should they know that it's thrown by a concurrent dictionary or should the stack trace lead them to Add(T) and not the TryAdd(TKey, TValue)? Which one is worse, validating an argument that will be validated by the wrapped class anyway, or letting a second exception to be initialized? — Şafak Gür, Apr 18 '13 at 06:58

score 7 · Answer 1 · answered Apr 17 '13 at 16:36

7

I would go with either this

public bool Add(T item)
{
    // This throws an argument null exception if item is null.
    return this.collection.TryAdd(item, 0);
}

or this

if (item == null)
    throw new ArgumentNullException("item", "Item must not be null.");

return this.collection.TryAdd(item, 0);

It depends on whether or not your class cares if there is a null.

If the only reason you are performing the null check is to avoid passing the null to TryAdd, then don't bother checking. TryAdd will do it's own checking and throw the exception.

If at some point you think you might use a different collection that allows null, but you still want your collection to not have nulls, then you should check yourself. This will protect you should that change happen at a future point in time.

Validation of parameters should always be the first thing a method does. There is no point in doing anything else if the parameters are invalid.

You should only catch an exception if you are going to do something with it. If you are just going to re-throw, or create a new expression that is equivalent, then don't bother catching it.

answered Apr 17 '13 at 16:36

cadrell0

17,109
5
51
69

+1 from me. I'd go with the second of those two myself - because the stack trace will start nearer the point of error (i.e. the code that passed null to the outer class's Add() rather than the inner collection's Add() which is kindof an implementation detail). Also I'd use `Contract.Requires(item != null)'`, but that's a whole different issue. – Matthew Watson Apr 17 '13 at 16:44
"If the only reason you are performing the null check is to avoid passing the null to TryAdd, then don't bother checking. TryAdd will do it's own checking and throw the exception." - exactly, but the `ArgumentNullException` it throws will have its `ParamName` property set to "key" since it's thrown by a `ConcurrentDictionary` which is the main cause of my doubt. I wrap `ConcurrentDictionary`, yes, but it's an implementation detail and I prefer not to expose that. For consumers it should just look like a thread-safe hash set that doesn't allow null values. – Şafak Gür Apr 18 '13 at 06:07
1

I would use the second option here to ensure the `ParamName` property is correct. I would also use `throw new ArgumentNullException("item");` because the extra message is an unnecessary duplication of obvious information (the single string argument to this `ArgumentNullException` constructor is the parameter name). – Sam Harwell Apr 18 '13 at 16:01

score 1 · Answer 2 · answered Apr 17 '13 at 16:43

1

I would say, what you should do rather depends on the effect you want to have. Do you want to swallow the error and not show it to the user? Dont re-throw the error in the catch box, but do include the try-catch. Do you want a custom error message, go with the Item == null check. There's not much use in generating a new exception instance, so that's rather out anyway.

As far as the rest of it goes..If you aren't logging the error, or handling it specially further upstream, then there's no need to re-throw the error after catching it. Otherwise, this comes down to personal style and whether you want a custom error message or not.

My favorite would probably be for the Item == null check with the custom error message, but that's rather because I like custom error messages. I find them more useful for me, but make certain that there is error handling around the thing that is calling this method, so that the error doesn't result in an unhandled exception further upstream.

answered Apr 17 '13 at 16:43

Nevyn

2,623
4
18
32

+1 for insight. So, to use a custom error message and hide the existence of the wrapped `ConcurrentDictionary` (which is just an implementation detail), you recommend using my own error handling *before* calling the dictionary's method and make sure it won't throw an exception. Then call it in a try-catch *just in case* and again, throw a custom exception from the catch block, right? The cause of my dilemma was the fact that dictionary already does this validation. So I thought maybe I should let it and if it throws an exception, throw a custom one instead of exposing that one. – Şafak Gür Apr 18 '13 at 06:20
Actually, you wouldn't need to call the add to the dictionary itself inside a try catch unless you really want to, its never really a bad idea, you might end up with a duplication error, it is a dictionary after all...but what I was referring to was outside of the shown method, when you call that method make sure you are inside a try catch block at some level, otherwise the custom exception you just threw will result in an unhandled exception error upstream. – Nevyn Apr 18 '13 at 13:37

score 1 · Accepted Answer · answered Apr 18 '13 at 15:52

What you should do depends upon what you want to document your class as doing. If you wish to document that an attempt to add a null item may fail in unspecified fashion, then simply make the call directly and let any exception bubble up. If you wish to document that you will return an ArgumentNullException with ParamName equal to item and do not wish to rely upon what the behavior of ConcurrentDictionary when it receives a null key, then you should check the argument before passing it to the ConcurrentDictionary. If you wish to document that your code will throw an ArgumentNullException with ParamName equal to item, but are willing to rely upon the ConcurrentDictionary to validate its arguments and throw an ArgumentException, and if performance is critical, another possibility would be:

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException ex)
{
    if (ex.ParamName == "key" && item == null)
        throw new ArgumentNullException("item", "Item must not be null.");
    else
        throw;
}

This code avoids any extra cost for parameter validation in the scenario where the parameter is not null (which should 99.9999% of the time be the case) but will nonetheless ensure that it will only claim to be the source of an ArgumentNullException in the scenario where such an exception occurs for the expected reason; in the event that a bug in ConcurrentDictionary causes it to accidentally pass a null argument to a method it calls internally even when it is given a non-null item to add, the above code will ensure that the original exception stack trace is not lost. Note that another possibility might be:

    if (ex.ParamName == "key" && item == null)
        throw new ArgumentNullException("item", "Item must not be null.");
    else
        throw new UnexpectedException(ex); // Probably a custom type

The basic idea being that if an ArgumentNullException escapes from ConcurrentDictionary.Add for some reason other than item being null, such an exception should not be caught by code that might expect an ArgumentNullException from you.

+1 and it's great that you have also addressed the multiple validations issue. Thank you. — Şafak Gür, Apr 19 '13 at 07:57
@ŞafakGür: It might worth mentioning that execution-time impact of parameter validation apt to be quite small, since the reference will likely have to be loaded into a register before it can be used, but code generation tools for .NET--rather than producing code to test for nulls before using references, it sets up the runtime environment so that a null dereference will trigger a hardware trap which then gets converted into a `NullReferenceException`. The fact that the Framework does that suggests that there's some performance advantage to avoiding null tests. — supercat, Apr 19 '13 at 15:15
Wow, thanks for the details. Never thought about its effects on performance before you mentioned it, tbh. My motivation was rather primitive, it just felt wrong to validate something and call a method that will do the same validation and throw the -almost- same exception I would throw but, good to know the specifics. — Şafak Gür, Apr 19 '13 at 15:30
@ŞafakGür: My personal preference would be to specify that routines should do what is necessary to uphold invariants even when fed invalid data, but that they are free to throw any exceptions they see fit *other than those which would be defined as meaning something else*. Java's checked exceptions are almost on the right track, except that specifying that unexpected checked exceptions should be promoted to unchecked ones should be no harder than specifying that they should bubble up to the caller (the former behavior is more often the correct one). — supercat, Apr 19 '13 at 16:05

Sten Petrov · Answer 4 · 2013-04-18T14:28:29.423

0

In your example they are all too similar but I'd go with the first option:

if (item == null)
    throw new ArgumentNullException("item", "Item must not be null.");

as it doesn't require a catch and looks more compact. It can also let you expand on the condition if requirements change and that won't add more lines of code, such as

if (item==null || item.Name == null)
    throw...

edited Apr 18 '13 at 14:28

answered Apr 17 '13 at 16:35

Sten Petrov

10,943
1
41
61

@ŞafakGür not the wrong example, my bad. TryXYZ usually keeps quiet about exceptions, but not in this case – Sten Petrov Apr 18 '13 at 14:29

Re-throwing the exception of a wrapped object

4 Answers4