[SERVER-70148] Document proper noexcept usage Created: 30/Sep/22 Updated: 29/Jan/24 |
|
| Status: | Open |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Andrew Witten (Inactive) | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Assigned Teams: |
Service Arch
|
||||||||
| Operating System: | ALL | ||||||||
| Sprint: | Service Arch Prioritized List, Service Arch 2023-08-21, Service Arch 2023-09-04, Service Arch 2023-09-18, Service Arch 2023-10-02, Service Arch 2023-10-16, Service Arch 2023-10-30, Service Arch 2023-11-13, Service Arch 2023-11-27, Service Arch 2023-12-11, Service Arch 2023-12-25, Service Arch 2024-01-08, Service Arch 2024-01-22 | ||||||||
| Participants: | |||||||||
| Description |
|
I think SERVER-50434 is a big deal. This gcc bug means that when we violate a noexcept specification, we have very few tools for figuring out where it came from. This happened in BF-26507 (an ongoing investigation) and in BF-17935. We get no log messages saying what exception was thrown. And in the case of BF-26507 (and possibly BF-17935), inlining makes the backtrace unhelpful.
In most of the cases where we use noexcept for "exception handling," we would be better off catching the exception, logging a message, and calling std::terminate.
I'm not sure how actionable this ticket is (feel free to close it as a duplicate of SERVER-50434), but I just want to bring people's attention to it.
|
| Comments |
| Comment by Jason Chan [ 13/Jun/23 ] |
|
billy.donahue@mongodb.com's suggestion that a minimal effort improvement would be to document about noexcept and its recommended usage patterns in the architecture guide. |
| Comment by Billy Donahue [ 07/Oct/22 ] |
|
We'd never ban it. noexcept is still vitally necessary for move operations, swaps, and things that really and truly won't actually throw. We just can't rely on the compiler to do the right thing when it's violated, is the thing. Noexcept does a few things, not all of which are broken. (1) It publishes a compiler-visible interface to a caller guaranteeing that the caller doesn't have to handle exceptions as a return path of a call. This allows algorithm selection and very theoretically some codegen optimization (particularly if the callee is not inline). (2) It sets up an implicit std::terminate try block around the body of the function with possible implementation-defined unwind elision. (3) A form of documentation. We would be using it SOLELY as a way of augmenting a function's signature, as in (1). It's much simpler to just not use noexcept at all for functions besides 1-ish liners. As for (3), I don't know how hot a take this is, but I don't think it's a good idea to use this keyword in lieu of docs. It doesn't allow future refinement of the API and noexcept is a forever kind of API decision that's unnecessary. It's also misused in our codebase to say that a function doesn't happen to throw, which is more a synopsis of implementation detail than a statement of contract. I think it can be harmful to readers because they don't know which meaning of noexcept is implied when they see it in code or are investigating BFs related to noexcept violations. |
| Comment by Billy Donahue [ 05/Oct/22 ] |
|
The underlying GCC bug is "UNCONFIRMED" upstream https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97720 Even the original GCC bugzilla bug report was sure to point out how dependent this behavior is on the exact arrangement of source code and compiler versions. In practice, we see that most of our terminate handler invocations do indeed get a current_exception, but it looks like it's possible for the wind to change direction and have that not be the case anymore for some failure paths. |
| Comment by Henrik Edin [ 05/Oct/22 ] |
|
Do we still have this GCC bug in the v4 toolchain? |
| Comment by Billy Donahue [ 03/Oct/22 ] |
|
-Wexceptions, being a static check, will only catch trivial cases where a throw occurs directly inside a noexcept function. It's not a substitute for the action proposed by this ticket. noexcept is good and necessary for move operations, without a doubt. As for performance, yes that's a consideration, but I'd say only for the most trivial and hottest of functions, like a trivial getter or other operators. Inlining these functions produces the same benefit. This ticket is meant primarily to work around a GCC bug that makes noexcept violations impossible to track down, in cases where a simple uncaught exception would be traceable. A noexcept mark is not a form of documentation. This trend has thankfully been halted, I believe. We have made a lot of progress toward backing out of unnecessary noexcepts over the last few years and should not be changing course without a serious proposal to do so. Lakos et al "Embracing Modern C++ Safely", after a lengthy chapter on noexcept, categorizes it as an "unsafe" feature, a category for features that have conditional utility, for sure, but a significant potential for causing harm on a large codebase through copycat overuse. A lot of thought has gone into this topic. |
| Comment by Alex Neben [ 03/Oct/22 ] |
|
I think the better solution would to add a compiler warning when throwing in a noexcept: https://clang.llvm.org/docs/DiagnosticsReference.html#wexceptions . There are good and legitimate cases for using noexcept for performance and clarity reasons. |