Bug 19602 – Under some circumstances: throwing Error subclasses unwinds without invoking dtors

Reproduction: import std.stdio; import std.traits; struct S { this(int) { writeln("this(int)"); } ~this() { writeln("~this()"); } } void dtorsOk() { with(S(1)) { throw new Error("Err"); } // dtor call generated here } // inferred as "nothrow" auto sneakyThrow() { throw new Error("Err"); } void failsToDtor() { with(S(1)) sneakyThrow(); // dtor call skipped here, incorrectly } // And this is the reason: static assert(hasFunctionAttributes!(sneakyThrow, "nothrow")); unittest { writeln("dtorsOk:"); try dtorsOk(); catch(Error) {} writeln("failsToDtor:"); try failsToDtor(); catch(Error) {} } Prints: dtorsOk: this(int) ~this() failsToDtor: this(int)

If "Errors" mean "bugs" - then it should not unwind the stack and allow catch() handlers to catch them and resume execution, as dtors will be skipped. If error throwing is fine, then "nothrow" should NOT imply no auto-generated exception handling code. It becomes useful only as a declaration of intent for the user, and not useful to the compiler. As long as this bug exists, there's a choice between these 2 options: * Errors should never really be caught - as they run in a completely broken state. * Destructors cannot be trusted in the general case. I see 2 possible fixes: * Ignore nothrow in the compiler codegen. Assume errors can always be thrown, even in nothrow (indeed they can). * Replace throwing/catching Errors with a different mechanism to signal/handle errors without incorrect stack unwinding.

This behavior is documented in the language specs since the very beginning of D2. In fact, there is currently a bug opened by Walter because dtors and `scope` statements are executed: https://issues.dlang.org/show_bug.cgi?id=17494 In addition, there is a PR to fix said issue: https://github.com/dlang/dmd/pull/6896 There is still value in catching `Error` though: as a last resort mesure: catch, log & abort is one thing, and catching at the top of a thread/fiber to avoid the program termination is another. Note that if `nothrow` applied to Error, the attribute would be essentially useless: `assert`, `new`, `switch`, slice casts and many other things could not be `nothrow`.

If you catch to *avoid* program termination -- you leak all destructors, leaving program in a likely-corrupt state. Note I did not suggest `nothrow` would apply to Error. Rather, I suggested that dtors *always* execute, even when Errors are thrown from `nothrow` functions. Or: replacing the corrupt-unwinding of Errors with a different error signaling mechanism. Currently, dtors *usually* execute, depending on arbitrary context. Catching Error (even unintentionally via catching Throwable) leads to cleanups in D being unreliable, and corruption being very easy.

Just to clarify on my last point, if "auto sneakyThrow" is changed to "void sneakyThrow" -- it DOES run the dtor! Because it is no longer inferred to be nothrow. This is a really unpredictable semantics for a simple program, doesn't feel at all reasonable in a language that desires safety.

(In reply to Mathias LANG from comment #2) > There is still value in catching `Error` though: as a last resort mesure: > catch, log & abort is one thing, and catching at the top of a thread/fiber > to avoid the program termination is another. For a unit test framework it's useful to catch AssertError to be able to continue running other tests.

(In reply to Jacob Carlborg from comment #5) > > For a unit test framework it's useful to catch AssertError to be able to > continue running other tests. Yep, however that's covered by the specs: > Individual tests are specified in the unit test using spec/expression, AssertExpressions. Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state. https://dlang.org/spec/unittest.html (In reply to Eyal from comment #4) > Just to clarify on my last point, if "auto sneakyThrow" is changed to "void > sneakyThrow" -- it DOES run the dtor! Because it is no longer inferred to be > nothrow. > > This is a really unpredictable semantics for a simple program, doesn't feel > at all reasonable in a language that desires safety. And that is the real bug, which is reported as 17494

(In reply to Mathias LANG from comment #6) > Yep, however that's covered by the specs: > > > Individual tests are specified in the unit test using spec/expression, AssertExpressions. Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state. > > https://dlang.org/spec/unittest.html It's difficult to know if the AssertError was thrown from a "unittest" block or from somewhere else.

(In reply to Mathias LANG from comment #6) > And that is the real bug, which is reported as 17494 That would be good for predictability. However, it makes unwinding quite useless, as opposed to a more sensible tls handler installed.

Spec-wise, https://dlang.org/spec/errors.html#the_d_error_handling_solution should be changed to clearly state what is happening. Currently, it says that throwing and catching Error is good for error handling but it very clearly isn't. It's contradicting object.exception and object.error page's advice, https://dlang.org/library/object/exception.html https://dlang.org/library/object/error.html, of never catching exceptions not derived from Exception (i.e. Throwables and Errors).

Assuming it is intended behavior to not run dtors when an Error is thrown: The Error documentation says that "Certain runtime guarantees may fail to hold when these errors are thrown, making it unsafe to continue execution after catching them." But why "after catching" ? What makes catching so special? User code will still run in (some) dtors. I think what the text is saying is that "Certain runtime guarantees may fail to hold when these errors are thrown, making it unsafe to continue execution PERIOD." Perhaps a good solution is to simply immediately abort execution upon throwing an Error.

The purpose of throwing an Error is to abort the program, but allow the programmer to catch it and execute some shutdown code. As mentioned earlier, scope guard will catch Errors, but it should really only catch Exceptions. This hasn't been fixed because there's existing code that relies on it catching Errors. The default behavior of generated code is to throw Errors on things like array bounds failures. This behavior can now be adjusted with the -checkaction switch: https://dlang.org/dmd-windows.html#switch-checkaction For existing compiled code, onRangeError can be used to set your own handler for array bounds failures: https://dlang.org/phobos/core_exception.html#.onRangeError Similarly, there's onAssertError, etc. Unittest failures at the top level do not call the assert fail handler, they call _d_unittestp(), which then calls the assert error handler. This means you can supply your own _d_unittestp() replacement. I believe this is sufficient flexibility in the handling of Errors. > Ignore nothrow in the compiler codegen. Assume errors can always be thrown, even in nothrow (indeed they can). Do not throw Errors when one expects RAII to unwind the stack. It will not do so reliably, and is not expected to. Throw Exceptions instead. Catching an Error does not mean the program is in a reliable state, and is only for the purpose of trying to catch a falling knife and hoping something of the program's state can be saved before aborting. > Replace throwing/catching Errors with a different mechanism to signal/handle errors without incorrect stack unwinding. I believe the above mechanisms give the programmer that option. I'm going to mark this as WONTFIX because it's working as designed and there are mechanisms in place to customize the behavior.

Walter, the question is: what benefits does the current approach have over a much safer approach of having *only* "onError" callbacks that allow the user to write some handling code in case of error before abort? 1) Even if the unwinding behavior is fixed to not unwind in case of errors, using "catch" blocks to handle errors has very surprising semantics: it runs code in broken contexts that have not run (scope(exit)s, finally's, destructors all skipped). 2) Worse still: the program easily continues after catching Errors, even if it is by accident -- in a completely corrupt state. These would remain 2 huge gotchas to know about when working with try/catch. Why keep them?

(In reply to Walter Bright from comment #11) > Unittest failures at the top level do not call the assert fail handler, they > call _d_unittestp(), which then calls the assert error handler. This means > you can supply your own _d_unittestp() replacement. Ah, I didn't know about that. Then `_d_unittestp` can be implemented to throw an exception instead of an error.