The Exception Mistake You Must Never Make in C#
HTML-код
- Опубликовано: 7 фев 2025
- Check out my courses: dometrain.com
Become a Patreon and get source code access: / nickchapsas
Hello everybody, I'm Nick, and in this video, I will show you one of the most common mistakes when it comes to exception throwing and explain why it's bad and how you can avoid it.
Workshops: bit.ly/nickwor...
Don't forget to comment, like and subscribe :)
Social Media:
Follow me on GitHub: bit.ly/ChapsasG...
Follow me on Twitter: bit.ly/ChapsasT...
Connect on LinkedIn: bit.ly/ChapsasL...
Keep coding merch: keepcoding.shop
#csharp #dotnet
First
FirstOrDefault()
Single
!First
FIFO
@@StuartQuinn 😄👍
A related mistake might be not to provide the inner exception when throwing a new exception.
throw new CustomException("Custom message", ex);
should also work without losing your stacktrace.
Edit: The thrown error at line in stacktrace will be CustomException line while 'throw' will keep the line where the error was actually thrown.
Right, but you should only do that if you have some useful info to add to that custom exception, otherwise you're only adding the need for the debugging developer to look at potentially multiple inner exceptions for no added benefit.
I sometimes do this when I'm working with *checked { }* blocks to give more meaning to the overflow exception by wrapping it in an ArgumentException, UnreachableException etc.
@@petrusion2827 even the name of your custom exception does add useful info without any additional data. You could easily add different logic handling your custom exceptions if need be.
I didn't know it isn't just syntactic sugar. This is neat and good to know.
Found out about this a while back, but it is a good point to make. It is easy to think that "throw ex;" is a smarter thing to do at that point of catching the exception.
A couple common ones I see:
Use stringbuilder instead of concatenation while building long strings.
Dealing with stuff as strings when there is a class for it. Like Uris, File Paths, Connection Strings, etc.
Using [EF|Database] queries in a loop, rather then querying for the set and looping through the set. Huge perf issues here.
I'm not sure I understand correctly. I would assume, it's better to ""download" (query) a bigger chunk from the database, then loop on it, then asking from a (possibly remote) database server several times each after each. What am I overlooking?
@@g3ff01No you got it. I was saying querying in a loop was bad. Query for all the rows and loop over the result set.
ExceptionDispatchInfo can help to capture exceptions that can be throw later. Also AggregateException is usefull to throw multiple exceptions toguether
What I am more interested in is WHY the stack trace erasure happens, and if so why Microsoft did not solve it?
When it comes to APIs, I made my own IExceptionHandler and IExceptionLogger implementations and registered them in the HttpConfiguration.
The former basically just returns a nice error message... unless you used a token with the aptly named 'CanSeeExceptions' permission.
The latter logs the request, with redacted parameters.
As for exception related 'problems', only one I have is that sometimes people make the scope of the function and the handler quite big.
And if you're missing line numbers, well then, you've got a null ref and a 1000 places it could be...
UNRELATED...
I'm sorry, but I laugh out loud every time I watch one of your videos...which are great, by the way! Thank you!
"Hello everybody, I'm naked...and in this video..."
Every time my wife comes by and hears me play one of your videos, she laughs too. We can't be the only ones who've mis-heard your name.
...but don't change your introduction on account of my dumb a$$. Your content, your presentation, your energy is awesome! ...and it's good to start the day with a laugh. Thanks again for taking the time to share all this great information.
I also saw many people still doing this mistake!
Thanks a lot Nick for the great content you provide man🙏
The other mistake is to catch “fatal exceptions” (e.g. OOM, threadabort, etc). The application can get in really bad state when people catch and ignore or try to do some kind of recovery on these exceptions.
There are some legitimate reasons for doing this - for example if you're designing a closed-source library and you don't want to expose details of your code structure or dependencies, but still need to throw an exception to the caller
In general, when many developers get something wrong, it's a sign something is not very natural or explicit about a language or framework. Ideally, the language should be clear enough that little to no explanation is needed to figure things out. For example, instead of doing "throw;", it could have been called "rethrow;", maybe (then again, reusing the same "throw" keyword is kinda nice, so I don't really know).
As for similar small things, I can't tell you how many times i've seen people misuse DataTables when checking query results. i.e. doing ToString()s and hard casts on datarows instead of dr("column_name"), and all the shenanigans with DBNull.Value.
What about Maybe i.e. TryFirst() vs T?
One mistake that I've encountered recently was having a try/catch around a function which returns an IEnumerable, if the enumerable isn't iterated on before the end of the try block, it will not be caught there.
What about passing ex as a parameter to a new Exception as an inner exception? It might be useful if you want to "change" exception type
That can work very well. It also allows you to have your code only throw one exception type, perhaps a custom exception, but still have that original exception available. I do that quite a bit when writing libraries. I want that original information to be available to the caller but still present an exception that is more in keeping with the level of abstraction of the library.
Perfectly fine if your intent is to add some extra context in the wrapping type that wasn't in the original exception. The original stacktrace is still there in the inner and it'll still be shown in the printout.
A somewhat related error I still see is people confusing when to use Debug.Assert() and when to validate parameters. For example:
public void SomeLibraryFunction(string myParam) {
Debug.Assert(null != myParam); //
Ahh, the perils of "Exception driven development." Throwing ex versus just throwing versus the very rare throwing new exceptions are one of the first things I had to make sure I got right in my early years working in dotnet. Another that's related to this was to actually try to be much more picky and explicit about the types of exceptions I'm catching so that I don't catch exceptions unnecessarily and end up hiding flaws in my code.
5:45 I see this all the time in a legacy codebase and I absolutely _hate_ it. The message is often the least useful part of the exception. The stacktrace is almost always the most useful, and usually the type of the exception after that. When rethrowing, either use a bare `throw;` to continue unwinding with the original exception, or wrap it as an inner exception and use the new outer exception to add additional context.
Additionally, there is *NEVER* a valid reason to throw the base System.Exception type. There's a lot of things where "never" should be treated as "almost never", but this one really is "never". Throw the most specific and applicable type for your error. There is zero benefit from throwing the base type, only downsides.
a basic mistake I see a lot of is just tossing null-forgiving `!` (or `null!`) on things without thinking about whether or not it should be used. I think it's partly due to some libraries not being great about NRTs (including Blazor! I really hope MS tidies this up!), which kind of "normalizes" null-forgiveness, or desensitizes devs to its importance.
What an exceptional video from you!
One thing I've seen way too often is people using some variation of ToLower or ToUpper to do case insensitive string comparisons. Not quite as big a problem as improper exception handling, but it makes me sad just reading the code 😕
8 minutes well spent. I have also done these mistakes in the past.
Thanks for the content!!! 😎😎😎
Thanks Nick
Actually I think that there is a situation where I'm totally fine with throwing a new exception. When I catch an Exception from another layer of abstraction and/or can enrich the exception with more information. But in this case I would never throw a raw Exception, it would always be a custom exception with the catchend Exception as inner Exception. Btw, I consider throwing a non custom exception a code smell.
Never underestimate the creativity of a developer, things could be even worse! I've seen code from a big tech company that was using a "SuccessfullyDoneException", just to break out of the routine when things were successfully finished.
I was already following the ReShaper suggestion for this, but thank you for clarifying the details.
ReSharper combined with SonarLint is a must-have for C# development imo.
Sometime you don't wanna let client know, what is real exception ;-)
of course we don't. but save the log for real exception for our investigation later.
Thanks for the tip!
Yeah, we had fun with this 10 years ago or so because throw ex was used all over the place.
The one I see a lot is people calling ToList on Enumerables when they don't need to.
I'd love to see an analysis of throw with global handler versus try/catch on everything and returning Problem or BadRequest. I hate the later, but I suspect it's faster.
Do you need it to be faster?
@@brandonpearman9218 It's one of those things where most of the time not, but you'll appreciate the money you save when you're under a DDOS attack and they're sending massive malformed requests that are causing memory and CPU overhead.
My issue is with logging. Yes, we log the exception object, but generally the message with use with the log is the outer exception message. If I'm looking at error logs (say SEQ), I want to see the actual exception from the inner exception.
Yes, this does break down when the inner exception has another inner exception, but this is a rare event.
False, logging the object should provide all of the necessary information unless you have some logging configuration that overrides it. You get only the outer message when you log exception.Message.
I tend to use my own helper method which takes an exception and builds and returns a string which I use for logging, i.e. includes the source, type, message and stack trace, and loops through all the inner exceptions. Also checks the type of each exception to do extra stuff as sometimes there is additional information you can get. For example, on an SqlException, you can get the list of errors thrown by the database with line numbers from the query from the SqlException.Errors array. I use that for things like sending email alerts and logging to a text file with Serilog. Haven't used SEQ before so can't say if this approach would play well with it.
One try/catch mistake I can think of is where to put your try/catch statements. I've seen people putting it only on the highest level and nowhere else. This means that if things change it becomes easy for the try to no longer catch the exceptions it was supposed to.
How can your entry point not catch(Exception ex)? If you have a try/catch on the entry point that has say catch(DivideByZeroException dzex) but doesn't have a catch(Exception ex) block, you're doing two things wrong not just one.
I learnt something new today.. noice 👌👌
A very good point to make.
Hi @NickChapsas I've seen on multiple places on the internet that his pattern of catching an exception, logging it and rethrowing it (log and throw), is considered an anti-pattern. Have you heard of it and what's your point on this ?
catch(Exception ex)
{
string message = ex.Tostring();
message = "";
}
This the actual code running on a 300+ places in an 20+ year old application i am refactoring for the past few months.
If you want to throw a new exception, do something like throw new ApplicationException(""Failed to retrieve weather", ex). That will make the original exception with all its stack trace an InnerException. (I just read @notbalding comment - he said it better)
The worst thing I've seen is somebody just logging ex.Message and not the whole exception. That gives you no context. And then they ignored the exception and let the code continue which would of course fail because something was null and then they'd just log the message about a null reference. Very frustrating to debug.
Thank you for the content 😊
Is throwing an exception or logging an exception better when it comes to dealing with exception handling in an API application?
For example while fetching an API response there could be some unforeseen exception and I don't want to pass those stack trace details to the consumer of the endpoint but at the same time will log the exception for the sake of traceability and debugging for the developer of the API . What would you recommend as an ideal approach?
Ideally you should log the exception, and return HTTP error from your API with some generic error message that doesn't expose any details. The developer will use application logs for analysis.
Wait, so what about throwing a new custom exception that wraps the caught exception? Like:
catch (Exception ex)
{
throw new FailedToRetrieveWeatherException(ex, city);
}
Where the custom exception inherits from Exception and simply passes along the inner-exception? Is important stack information lost here?
Nick do you recommend using your Cosmonaut library at this time? It has not been updated in a little bit and I was curious as to whether you think it is still good to use.
Interesting. Thank you! :)
There's an even better, although uglier, way of doing this: by using an exception filter to call a function that always returns false. This way you get a chance to log the exception without actually "catching" it.
Nick what about
ExceptionDispatchInfo.Capture(ex).Throw()?
Is it different from just throw?
That's exactly the thing that produces the "stack trace from previous location" section. It's used behind the scenes in async/await, because it needs to capture an exception and rethrow it in a completely different context (possibly other thread), so "throw;" doesn't apply in that case.
An interesting mistake can be using Thread.Sleep instead of Task.Delay in async code.
It's 2023 and I, too, am still seeing this issue in a lot of code these days. Thanks for calling this out in your vid.
Great information, didn´t knew that! What about you want to throw a different exception with more informations, such as paths, names, etc.? How do i pass down the full original information, so that the stacktrace is preserved? new MyException("bar whatever foo"", ex);
Let's say you're catching this exception and outputting it to a log file, after you log the exception you created, you then need to "while (exception.InnerException is not null)" 1) log the exception then 2) exception = exception.InnerException. So you log all the Inner exceptions, even inner exceptions to the one that wasn't thrown by you, since they could contain additional helpful info. For example an exception that was thrown when sending an http request may have a SocketException as the inner exception. The inner exceptions will have the full original information.
Not sure I understand why would you want more data then actually needed? Stacktraces can be confusing and tough to wadw through and not sure why adding even more information helps.
Hi nick …can you make a video talking about ( Ocelot ) and load balancing APIs
Nick, did you ever do a video on using error details for returning information from HTTP endpoints?
Without ExceptionDispatchInfo.Capture(ex).Throw() this video doesn't cover the problem entirely.
Plus it good to mention that throwing a new exception with passing original exception as inner exception is also good.
Should you use catch "SpecificException" or catch Exception when SpecificException?
IL seems to be bigger using when, so my assumption would be to multiply your catches
Always catch the most specific type you intend to handle. If you intend to handle a larger scope, then that's what you should catch. For example, if you're at the top of the application stack and your intent is to handle an "if all else fails" situation by logging, showing an error message, and gracefully closing, that's a perfectly valid reason to catch a base type. But if your intent is to handle (for example) an InvalidOperationException, than catch just the InvalidOperationException. Just catch what you intend to handle, so you don't accidentally try to handle what you aren't able to. Otherwise you'll end up hiding errors you didn't expect.
And if you're worried about performance, keep in mind that any exception still makes a sizeable performance hit due to constructing the stacktrace, so if you're able to error-handle proactively before an exception would otherwise be thrown, do so.
But shouldn't the ex object contain all the previous context? (I'm not asking if it does, only if it *should*.)
In my opinion this is C# design flaw, because it's intuitive to throw something rather than just throw; And if I throw the same exception it turns out that behind the scenes C# throws a new Exception from the same place we throw it. That's why previous stack trace is missing. As a workaround we can throw new Exception and pass original one within InnerException property - that at least Microsoft could implement behind the scenes.
The stack trace is built when you do the throw, and not when you create the exception. Throwing a variable doesn't mean it's referring to a new or an existing exception, so it will insert the current stack trace anyway. To not have this problem, c# would need to expose the "GetStackTrace" so devs can put it themselves in the exception and not have it added on the throw.
Do other languages behave the same? Php for example?
_logger.LogError(ex, "") will not record the original stacktrace? how can i capture the original one in logger with only throw?
A rare occasion where I see one of your videos that says "Don't make this mistake" and I'm not making it :)
most static code analyzers pick it up, AFAIR even the default one in VS
An 8 minute video for a 60 second explanation...
Don't return void from async methods
throw new FirstCommentException("Zaaamn");
And also if you catch TargetInvocationException, don't throw ex.InnerException, but instead ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
As I remember there already was a video from Nick about this problem 🤔
It was a short
Only a problem if the exception originated from your code. If it's from a library code, then you're not missing out on much (unless the library is poorly developed).
Only if you're okay with not reporting potential bugs to the developers of libraries you depend on
@@jtrc19953 The bug can be detected from a highier level. I'm not going to stop work to track down a library code bug.
For anybody interested - logging and immediate re-throwing the same exception is an anti-pattern.
Logging should be part of handling the exception, and in here you're not handling it.
Frequently, the caller will handle the exception by logging the error as well, and you'll end up with the same error logged multiple times.
People do this, because they don't want to learn who is handling / should handle the exception and think it's better to be safe than sorry. A very lazy attitude that can easily lead to log spam.
You are right, however I can list you at least one reason why logging multiple places could be useful: some context information may be only at certain places which you also want to log however you don't want to loose the original exception itself
@@novak-peter You can always have the original exception as the inner exception. You would create a new instance of a more specific exception, add the context information to it, and provide the original exception as the inner.
This way, you don't lose any context information and have the original exception available for the exception handling / logging.
var task = DoSomethingAsync();
vs
var task = Task.Run(DoSomethingAsync);
Might be the case
Thank you. I knew that you could do "throw;" or "throw ex;", and that the analyzer suggests the former. But I didn't ever read why the analyzer suggested that. I always assumed that it was simply a style suggestion.
@nickchapsas nice haircut btw 🙂
Well…Jokes aside but if Nick said not to do something then we should take heed of what he said and never do it.
7:06 - Justification: "I'm making a video"
Got it. Never develop a weather app. Good advice indeed.
Another common mistake: `async void`
Please do a video on ConfigureAwait()
i don't throw exception any more 😅
I love you ❤
This is also checked by CA2200.
I predicted that this will be the case 😅
I was wrong !
A small thing many do not understand is to avoid nested scopes if possible.
Obvious thing
Another common misconception I see is with the lazy'ness of LINQ queries, and that most LINQ expressions are just attaching a set of processing instructions to an existing collection.
This is why F# has raise and reraise. C# should have had throw and rethrow.