Add note about catching exceptions in C++

[DrusTheAxe]

Added best practice about catching C++ exceptions and HRESULTS per @kennykerr from this thread

[riverar]

Trying to catch/handle std::bad_alloc doesn't seem right. What are you going to do when it's caught? Try to call some telemetry code and/or alloc memory to report the memory allocation failure?

[DrusTheAxe]

Try to call some telemetry code
Sure. Not every allocation is 4 bytes, and if I fail to allocate memory deep in some callstack when I boil up N layers and unwind work along the way I very likely have freed up memory so logging isn't a pointless deadend.

I only see 2 choices for outofmemory: immediately terminate the process, or handle it like any other error. The former is egregiously painful (for support as well as the user). It's not hard to write code that degrades gracefully (if you're willing to) -- this action failed due to out of memory but it doesn't guarantee the next one will.

If you're catching exceptions and want an HRESULT, it's either because you're...

1. ...logging (or telemetry or the like)
2. ...returning an HRESULT
3. ...doing conditional processing on some specific HRESULTs

E_OUTOFMEMORY is important for 1 (as anyone who's ever done support or troubleshooting knows)

It's useful and in some cases required for 2 e.g. can't throw exceptions out of a Flat-C API or across a COM/WinRT boundary, or across modules for that matter.

3 is a little fuzzier. It's hard to imagine when you'd want to detect E_OUTOFMEMORY. In that case perhaps just catching the HRESULT is good enough. But then if the HRESULT doesn't match something you're probing for do you swallow the exception? I hope not. There's lots of errors that could happen below you (storage errors, etc) that are 'environmental soup' that don't interest you but shouldn't be swallowed and hidden from higher layers. One hopes if you catch an exception probing for specific errors and it's not one of them, you rethrow the exception.

Is the catch(...) guideline bad in case 3? We're already into exceptional cases so perf isn't an issue.

There's merit to having fewer rules and simplicity of coding patterns - makes it easy for devs to do the right thing and harder to do the wrong. Is that not warranted here?

[DrusTheAxe]

BTW this caught my eye because the notification code caught hresult, probed for certain conditions and match or not, swallowed the exception and added the hresult to a ChannelStatus(?) object. No hresult exception flowed past that.

Except E_OUTOFMEMORY via std::bad_alloc.

An example of case #2 but handled incompletely.

(This issue had come up in other circles a couple of months ago but the notification code is what pushed me to propose this PR)

[kennykerr]

In hindsight it probably wasn't worth the effort to distinguish bad_alloc from other failures in C++/WinRT, but what I suggested in the other thread was not meant to be a best practice. It's what you do when you are dealing with an unfortunate API design that you must work around, either the one you're implementing or the one you're calling. Ideally, you would not need exception handling to deal with various expected failure conditions. What's missing is a TryXxx variant of the API that doesn't throw an exception. In that scenario, you don't need to think about exception handling at all and C++/WinRT automatically deals with error propagation for you.

[riverar]

If you're catching exceptions and want an HRESULT, it's either because you're...

1. ...logging (or telemetry or the like)
2. ...returning an HRESULT
3. ...doing conditional processing on some specific HRESULTs

E_OUTOFMEMORY is important for 1 (as anyone who's ever done support or troubleshooting knows)

How do you log E_OUTOFMEMORY without memory for the string? Seems you've built up a bunch of rules around the assumption that "my alloc request might be smaller than the original one that failed".

[kennykerr]

Note that if the implementation of the API is using C++/WinRT then any uncaught exception is automatically turned into an HRESULT at the ABI boundary. WIL also supports hooking this ABI boundary and providing additional logging/telemetry if needed. You're better off not writing any exception handling code yourself.

[DrusTheAxe]

C++/WinRT is smart about it (yay) but WRL isn't. Until C++/WinRT can create OOP servers WRL's not going away. And C++/WinRT is N/A for Flat-C APIs.

@BenJKuhn you beat me to it. I was thinking the same.

And anyway in the notifications case it's not an ABI boundary, it's getting the HRESULT to return later. Stashed in an attribute for later access. A "deferred return HRESULT" if you will. Unless it's E_OUTOFMEMORY then it's missed.

The question still stands: if you're catching HRESULT exceptions is the pattern in this PR a good practice we should be advocating?

[benstevens48]

If you only want to catch exceptions coming from an HRESULT, presumably thrown via check_hresult, maybe it would be best to define a variant of check_hresult (and possibly throw_hresult) that only throws hresult_error exceptions and not std::bad_alloc. (I would actually be in favor of changing the behavior of check_hresult and throw_hresult to do this, but I don't know if that would be considered).

On the other hand, if you need to catch all exceptions including std exeptions, then the above snippet is probably required. But I imagine that usually a std exception would indicate a bug in the code.

(As an aside, I initially assumed that check_hresult would only ever throw hresult_error exceptions and have used this assumption liberally throughout my code. I happen to be doing image processing using WIC and D2D, so not having enough memory does not necessarily mean that the app should crash, because it may just be that the image is too large. At some point I am probably going to have to go through all my code and address this.)

[DrusTheAxe]

std::bad_alloc indicates an environmental problem. Nothing a developer can code to prevent.

We may want to enhance check_hresult or other APIs but this PR is focused around the coding guidelines du jour. We can always refine it later if/when check_hresult is changed or other improvements are available.

That is a key reason for Reunion? That we can iterate quickly.

[kennykerr]

And anyway in the notifications case it's not an ABI boundary, it's getting the HRESULT to return later.

That's a different situation for which my targeted suggestion is not applicable. If you're storing it for later you should store the exception - e.g. std::current_exception() - rather than just the error code otherwise you're leaving the error information - e.g. IErrorInfo - behind. Handling the ABI boundary yourself is hard.