I am confused because in quite a few places I've already read that the so-called 'boneheaded' exceptions (ones that result from bugs in code) are not supposed to be caught. Instead, they must be allowed to crash the application:
- Vexing exceptions, by Eric Lippert
- A comment under Eliding Async and Await, by Stephen Cleary
- Answer below Is it a good practice to use self-defined exception?, by Draco18s no longer trusts SE
At least two of the three above people are established authorities.
I am surprised. Especially for some (important!) use cases, like server side code, I simply can't see why is catching such an exception suboptimal and why the application must be allowed to crash.
As far as I'm aware, the typical solution in such a case is to catch the exception, return HTTP 500 to the client, have an automatic system that sends an emergency e-mail to the development team so that they can fix the problem ASAP - but do not crash the application (one request must fail, there's nothing we can do here, but why take the whole service down and make everyone else unable to use our website? Downtime is costly!). Am I incorrect?
Why am I asking - I'm perpetually trying to finish a hobby project, which is a browser based game in .net core. As far as I'm aware, in many cases the framework does for me out of the box the precise thing Eric Lippert and Stephen Cleary are recommending against! - that is, if handling a request throws, the framework automatically catches the exception and prevents the server from crashing. In a few places, however, the framework does not do this. In such places, I am wrapping my own code with try {...} catch {...}
to catch all possible 'boneheaded' exceptions.
One of such places, AFAIK, is background tasks. For example, I am now implementing a background ban clearing service that is supposed to clear all expired temporary bans every few minutes. Here, I'm even using a few layers of all-catching try
blocks:
try // prevent server from crashing if boneheaded exception occurs here
{
var expiredBans = GetExpiredBans();
foreach(var ban in expiredBans)
{
try // If removing one ban fails, eg because of a boneheaded problem,
{ // still try to remove other bans
RemoveBan(ban);
}
catch
{
}
}
}
catch
{
}
(Yes, my catch
blocks are empty right now - I am aware that ignoring these exceptions is unacceptable, adding some logging is perpetually on my TODO list)
Having read the articles I linked to above, I can no longer continue doing this without some serious doubt... Am I not shooting myself in the foot? Why / Why not?
If and why should boneheaded exceptions never be caught?
RemoveBan()
would probably be described as an example of a "vexing exception", where its failure shouldn't actually throw an exception. The article gives an example of howint Int32.Parse(string)
used to throw when it failed to parse anint
from a (typically user-provided)string
; this was a poor design choice, and it was later fixed to bebool Int32.TryParse(string, out int)
, which instead of throwing on failure to parse, wouldreturn false;
. Likewise, you'd probably wantTryRemoveBan()
. – Nat Jan 04 '20 at 18:43RemoveBan
'sdb.SaveChanges
succeeds, so we have an exception that I guess qualifies as exogenous and should be ignored. Boneheaded exceptions fromRemoveBan
are, however, still a problem – gaazkam Jan 04 '20 at 18:49// TODO
comments in those empty catch blocks. – RonJohn Jan 05 '20 at 05:20// TODO
comments in either. You never actually get back to properly handling either. (I was just recently burned wasting about 2 days in legacy code that swallowed a NPE in an empty catch block.) – Dave Rager Jan 06 '20 at 00:15//TODO
is to avoid committing them. It's not a very strict policy but the idea is that I'd place copious//TODO
comments but I don't want them in the final released code. Either actually do them or remove them. Removal should be accompanied with a issue tracking ticket or just understanding that this will likely never be done (e.g., unneeded feature). I do have some//TODO
comments left out in the code base even after that but the number is generally low. I'd prefer to be even stricter than this but...it's not usually worth it for me. – VLAZ Jan 06 '20 at 06:48//TODO
s are finished? – Tvde1 Jan 06 '20 at 12:00TODO
- no point in letting code rot on my machine for more than a few days without a push. As I said, it's not a very strict policy. – VLAZ Jan 06 '20 at 12:02assert()
to purposefully, and immediately crash the process, without giving any other part of its code any chance of recovery. Boneheaded at its best. The effects of this are: Any error that triggers anassert()
will be considered by a human, the programmer inspecting a failedassert()
gets directly pointed at the spot where the error was detected, and programmers will work hard to make sure noassert()
is ever triggered. This ensures that the process will never work with corrupted data that wouldn't pass theassert()
calls in the code. – cmaster - reinstate monica Jan 06 '20 at 20:49GetExpiredBans
andRemoveBan
? The assumption here seems to be "these can fail arbitrarily at any time and we can always ignore that failure". OK, if that's the case then put the try-catch inside them and then the caller doesn't need them. Why did you decide to put the try-catch in the caller, rather than the callee? – Eric Lippert Jan 06 '20 at 20:55ExecuteAsync
code intry
/catch
- though probably I should remove the innertry
/catch
– gaazkam Jan 07 '20 at 10:59try
/catch
is for; but having read your text about 4 exception types I was understanding that you were arguing to crash the server and incur downtime in the service until the problem is manually fixed. So I got confused and asked this question. – gaazkam Jan 08 '20 at 22:38try
/catch
... I think the answer to your question is that this granulity is to break each operation into subtasks such that no tasks fails because the other one failed - just like in case of processing requests: they should be independent from each other, so failing one request shouldn't entail failing any other request (as would happen if the server died). – gaazkam Jan 08 '20 at 22:38