Skip to content

Ensure no exceptions from dtors; marked noexcept(true) in C++11#538

Closed
ghost wants to merge 1 commit intopocoproject:masterfrom
telerik-boneyard:master
Closed

Ensure no exceptions from dtors; marked noexcept(true) in C++11#538
ghost wants to merge 1 commit intopocoproject:masterfrom
telerik-boneyard:master

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 12, 2014

This is a rather extensive change. Here's the background:

In C++11 destructors are implicitly specified as "noexcept(true)". This means destructors are not expected to throw exceptions and that std::terminate() will be called instead if they do. And std::terminate() will abort and kill the process, terminating with an error message. Not desirable.

http://en.cppreference.com/w/cpp/language/destructor

"If no exception-specification is provided, the exception-specification is one that that would be used by the implicitly-declared destructor (see below). In most cases, this is noexcept(true)."
"the exception specification of the implicitly-defined destructor is noexcept(true)"

http://en.cppreference.com/w/cpp/language/noexcept_spec

"If a search for a matching exception handler leaves a function marked noexcept or noexcept(true), std::terminate is called immediately."

I came across this issue when upgrading a medium codebase to use C++11 on MacOS, which is now the default since XCode 5.0.1. The app uses most of the Poco libs. At exit() the static variables in Poco (Mutex etc) would be destroyed first and my app's background-threads would then afterwards cause std::terminate() to be called because destructors used there would throw exceptions when those presumably always-available Poco resources no longer worked; e.g. locking a Mutex would now fail. It took me several days to pinpoint this problem and it involved many Poco-classes.

Because instances of any Poco-type can be alive upon exit, the only reliable and maintainable solution is to ensure that no Poco destructors can throw exceptions. Prior to C++11, avoiding destructor-exceptions was merely strongly recommended, but C++11 effectively mandates it - as in, "if your dtor throws the app will die".

That is what this fix contains.

As guideline I've chosen to guard any destructor-code that was not immediately obviously fail-safe. Destructors solely operating on the object's primitive members (doing count-- or calling delete, for instance) do not need guarding, on the assumption that the code is correct and the heap not corrupted. By any destructor that calls into unknown territory, i.e. other methods, have been wrapped into an exception-handler. Many destructors already had a suitable exception-handler and they have been left alone.

I realize this is a big change-set. And careful analysis may reveal that some destructors may not need it. However, cherry-picking which to fix seems hazardous at best. So, even though it's overwhelming I believe the alternative, having Poco potentially indirectly terminate the app, is worse.

@obiltschnig
Copy link
Copy Markdown
Member

I'm not really happy with that kind-of brute force approach. The problem with this is that it tends to hide errors in an application. Your issue of global objects being destroyed while some threads rely on them is such a case, and making destructors silently swallowing exceptions merely hides the issues instead of solving them properly. There are some destructors where it may be OK to silently swallowing exceptions (e.g., stream buffers that flush, which may fail due to an I/O (network) error), but the majority of the destructors will only throw if something really serious goes wrong. The fact that e.g. a Mutex someone's trying to lock or unlock is no longer available is such an issue. Another issue is that on Windows, a catch (...) will also swallow access violations and similar errors. That solution may work in your particular case, but I'm not comfortable with mandating that for all users of POCO.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 17, 2014

Trust me, I'm not happy either with this kind of brute-force boiler-plate code.

But - I do believe it's necessary for C++11.

Presumably it was a somewhat controversial decision in C++11 to mandate that std::terminate() be called when noexcept(true) is violated, but it's in the standard now and we have to cope. I think this new behavior will come as a surprise to many. I was quite surprised myself.

The recommendation from "everyone" is to abide by this rule. "A destructor shouldn't throw", as Stroustrup write in his C++11 FAQ at http://www.stroustrup.com/C++11FAQ.html#noexcept

In reality I see no other option for Poco than to honor the promise that destructors don't throw. What other maintainable options are there? Cherry-picking based on the current code-base seems terribly brittle. I much prefer enforcing a sweeping rule: no destructors may throw. It's usually the right and necessary thing to do and it's easy to verify confidently just by looking at the code; analyzing possible exceptions from method-calls isn't. And tracking down a bug caused by a static member of the Mutex-class no longer existing after exit(), so lock() fails and cause some other destructor to terminate the program without any trace of the issue on the stack, because it jumps straight to calling terminate instead of raising an exception - well, let's just say it wasn't very easy to track down. And after having fixed it for one Poco-class, a similar error popped up from another. And another. That's when I chillingly realized the problem was very broad, especially after exit().

As for detecting bugs: a best practice, which I find very reasonable, is to only have destructors call into other methods instead of containing code of their own, so to speak. That's how Poco usually is written anyhow. Those methods can then be invoked explicitly during the course of the app and tested on their own. So eg method close() may surely throw exceptions when called explicitly and when tested, but when invoked from within a destructor then any exceptions should be hidden.

I know that a changeset of 167 files at first glimpse seems like an overkill, almost regardless of the cause :-) Surely so many changes cannot be needed, ever. There must be some other way. But I can't see any other robust way of handling C++11 (other than changing the default behavior of noexcept or marking all destructors noexcept(false) for C++11 only, which seems even worse). Can you?

@PureAbstract
Copy link
Copy Markdown

Presumably it was a somewhat controversial decision in C++11 to mandate that std::terminate() be called when noexcept(true) is violated,

To the best of my recollection, that wasn't where the controversy lay. What was controversial was the decision to make destructors noexcept(true) by default - and I argued strongly against this in committee. If we'd done that back in '98, then I'd have been fine with it - but we didn't, and now we have the kind of backwards compatibility problem that we're seeing here. (I know people who daren't move their code to C++11 precisely because of this change)

The recommendation from "everyone" is to abide by this rule. "A destructor shouldn't throw", as Stroustrup write in his C++11 FAQ at http://www.stroustrup.com/C++11FAQ.html#noexcept

Indeed. And it's been a recommendation for a long time - certainly pre C++11. And it's generally good advice (throwing destructors are Bad News during stack unwinding). The change here is that a throwing destructor can now call terminate when you're not unwinding... sigh

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 17, 2014

What was controversial was the decision to make destructors noexcept(true) by default

Seems reasonable - I'll take your word for it. This detailed blog post referred to commitee meeting minutes that includes passages like

there had been a great deal of discussion on these issues, which boiled down to
"what happens when you say you won't throw, and something throws anyways." In the
end, the group decided to use the direction of calling terminate in such a case,

so the choice to invoke terminate() also raised some concerns in itself.

This behavior is definitely a major issue when moving to C++11.

Andy, would you have some further recommendation on how to cope with this for the Poco project?

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 18, 2014

I found the relevant meeting minutes here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3166.html. It reasons:

This will serve to call attention to these destructors, where they can be modified,
either with a throws() or noexcept clause, or a redesign. This will result in clearer code
that terminates less often.

"We'll make your code crash more often so you'll notice and clean it up and it eventually will crash less often". Well, it worked: I surely noticed :-)

So, other options include marking destructors throw(...) or noexcept(false). The former is deprecated in C++11. The latter is an unknown keyword in pre-C++11 so it would need #ifdef or be a #define, both of which feels kind of clunky.

Alternatively, Poco could retract support for C++11. In it's current state it's very vulnerable and prone to cause applications to crash, which is surely not desirable for such an excellent high-quality library as Poco.

I personally find both two options less attractive than exception-proofing the destructors. If that means adding try-catch boiler-plate code to every destructor for consistency then yes, so be it. It's clear, it's safe, and it works.

@obiltschnig
Copy link
Copy Markdown
Member

Since there's no way around the catch all block, I'm strongly for at least reporting exceptions caught this way. For that purpose I've added the poco_unexpected() macro, which is to be used in such handlers. In debug builds it will log the exception and break into the debugger, if available.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 23, 2014

The poco_unexpected() macro sounds like a good idea.

I can see your changes only include 71 files while mine touched 167 files. Some were likely not needed, like when the code called by the destructor by inspection can found to be harmless. E.g. when calling release() which in turn just deletes something. But if find it a bit hard to reason about the safety for some others.

For instance, the destructor of ParallelSocketReactor.h does this:

this->stop();
_thread.join();

and ThreadImpl::JoinImpl() for POSIX threads is implemented like so

void ThreadImpl::joinImpl()
{
    if (!_pData->started) return;
    _pData->done.wait();
    void* result;
    if (pthread_join(_pData->thread, &result))
        throw SystemException("cannot join thread");
    _pData->joined = true;
}

It seems possible that this could fail and throw.

@obiltschnig
Copy link
Copy Markdown
Member

I have generally not fixed test suite destructors. If those fail we'll notice anyway. Have gone over all destructors again today and fixed a few more. Hopefully we've got all now.

@obiltschnig obiltschnig added this to the Release 1.5.4 milestone Oct 6, 2014
@obiltschnig obiltschnig closed this Oct 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants