Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add note about catching exceptions in C++ #514

Merged
merged 4 commits into from
Mar 10, 2021
Merged

Add note about catching exceptions in C++ #514

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25f261a
Add note about catching exceptions in C++
DrusTheAxe Mar 3, 2021
25401da
Minor formatting improvements
DrusTheAxe Mar 3, 2021
b5005e6
Added C++ tagging to a code snippet
DrusTheAxe Mar 4, 2021
26ff5fe
Incorporated feedback
DrusTheAxe Mar 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions docs/Coding-Guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,31 @@ Project Reunion prefers using industry-standard coding styles, guidelines, and p

#### C++

Project Reunion implementations prefer C++ and C++/WinRT implementations. New code must adhere to the **cppcoreguidelines** at
Project Reunion implementations prefer C++ and C++/WinRT implementations. New code must adhere to the **cppcoreguidelines** at
https://github.com/isocpp/CppCoreGuidelines and be /W4 clean (for Visual C++.)

Prefer existing std:: and gsl:: types, but use **wil** (https://github.com/Microsoft/wil) types for Windows-specific helpers and lifecycle management
Prefer existing std:: and gsl:: types, but use **wil** (https://github.com/Microsoft/wil) types for Windows-specific helpers and lifecycle management
rather than creating your own.

##### Catching Exceptions and HRESULT

Don't just catch `winrt::hresult_error` or other variations as that doesn't catch `std::bad_alloc`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@DrusTheAxe DrusTheAxe Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@riverar riverar Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Use the following snippet to catch exceptions and retrieve their HRESULT:

```c++
catch (...)
{
auto e{ winrt::hresult_error(winrt::to_hresult(), winrt::take_ownership_from_abi) };
...e.code() contains the WinRT exception or best-guess conversion from a C++ exception...
}
```

#### Markdown

**GUIDELINE:** The preferred line length limit is ~100 characters. GitHub formats lines regardless of individual length but GitHub diff is line oriented.
**GUIDELINE:** The preferred line length limit is ~100 characters. GitHub formats lines regardless of individual length but GitHub diff is line oriented.
Keeping lines within the preferred limit makes changes easier to review.

#### Other Languages

If new languages become common, we will describe the coding guidelines for such languages here.
If new languages become common, we will describe the coding guidelines for such languages here.