Re: [lmi] PATCH: use std::uncaught_exceptions()

[Greg Chicares]

On 2018-03-25 16:40, Vadim Zeitlin wrote:
> On Sun, 25 Mar 2018 16:10:24 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-03-24 22:34, Vadim Zeitlin wrote:
> GC> > On Sat, 24 Mar 2018 21:19:34 +0000 Greg Chicares <address@hidden> wrote:
> GC> 
> GC> [...lmi must never produce an invalid illustration--i.e., PDF file...]
> GC> 
> GC> > GC>  - throw in the dtor when not unwinding, even though that goes
> GC> > GC>    against accepted pre-C++17 wisdom?
> GC> > 
> GC> >  I guess we should indeed use LMI_ASSERT() and throw from the dtor. It 
> also
> GC> > seems that we need to delete the PDF output file, if it exists, if an
> GC> > exception was thrown.
> GC> 
> GC> Let's establish the premises first.
> GC> 
> GC> (1) AIUI, PDF files are generated something like this:
> GC>   (a) create PDF file for output
> GC>   (b) append stuff: b1, b2, b3...
> GC>   (c) close the file
> GC> and it's my impression that any error in (b) leaves a file that may
> GC> be syntactically valid. For example, I've seen the wxPdfDoc code
> GC> create PDF files that can be opened in a PDF viewer but appear to
> GC> have no contents. I think this is why you say above:
> GC> | seems that we need to delete the PDF output file, if it exists, if an
> GC> | exception was thrown.
> GC> but correct me if I'm wrong.
> 
>  You're correct, but I've actually realized that uncaught_exceptions()
> could be really helpful here, as the PDF file is actually written to when
> wxPdfDC::EndDoc() is called and we currently do this in pdf_writer_wx dtor
> unconditionally. We could, however, check whether we're in the process of
> stack unwinding and not do it then, which would avoid touching anything on
> disk in case of an error.

What I object to is implementing part of the task in a dtor.
Using uncaught_exceptions() here would be a refinement of
what I consider to be a technique we should avoid.

>  Alternatively, and more verbosely, but also less magically, we could just
> stop calling wxPdfDC::EndDoc() from pdf_writer_wx dtor automatically and
> require the code using this class to call some close() method explicitly.

Yes, let's do that, please.

> I suspect that you prefer this solution, but I'd still like to check for
> uncaught_exceptions() in wxPdfDC dtor to assert (or warn?) if close()
> hadn't been called without an active exception, as this would be a logic
> error in this case.

The perceived advantage of implementing the final phase of the task
in a dtor is that the dtor is certain to be called at the right
moment. IOW, if we don't implement a separate close() function at
all, then we can't fail to call close() in the right place along
every possible path of execution.

The practical problem with implementing the close() logic in a dtor
is that it introduces fragility: if an exception is thrown by a dtor
that was called as part of stack unwinding (due to a different
exception elsewhere), then the application terminates abruptly.

But please review branch 'odd/dtor-verifies-postcondition', where
I believe I've done what you recommend above, for the progress_meter
class. The dtor notifies us whenever we've followed an execution
path that doesn't call culminate() as it should (which would be a
logic error).

>  To summarize, my preferred solution would be to continue calling EndDoc()
> from the dtor automatically, but skip doing it during stack unwinding. But
> if you prefer the other one, as I suspect, I could live with it as well.

Yes, I do strongly "prefer the other one" as you suspected.

It had been my ambition to demonstrate that, if we were to change
~numbered_page() so that it throws an exception:

  if(extra_pages_ && !std::uncaught_exceptions())
    {code_that_should_throw_an_exception();}

then an error in the implementation of the {} body would do
something undesirable. If I had been able to demonstrate that,
then that might prove that we shouldn't do it. What I instead
demonstrated is that I'm not even capable of writing this sort
of code: my attempt was erroneous, but not in the way I had
intended--though I had studied the standard and 'n4152.pdf',
along with many online articles. I thought I was pretty good
at tricky stuff, but "even Cathy Berberian knows there's one
roulade she can't sing", and one of my limitations is that I
can't write a deliberately-throwing dtor reliably--which is
okay, because, this abortive demonstration aside, I wouldn't
want to.

> GC> (2) For a long time, "never let exceptions escape from destructors"
> GC> has been the usual advice, and our production system has always
> GC> followed that "best practice". C++17 adds std::uncaught_exceptions(),
> GC> which lets us throw from dtors safely under certain circumstances.
> GC> Someday I'll probably learn enough about this to become comfortable
> GC> with it, but that will take time and effort. Maybe now is the time,
> GC> or maybe not; but until I invest that effort, accepting this new
> GC> idiom into production is an avoidable risk, and I am risk averse.
> 
>  Yes, I'm uncomfortable with this too.

Consider what happens here if EndDoc() throws:

pdf_writer_wx::~pdf_writer_wx()
{
    // This will finally generate the PDF file.
    pdf_dc_.EndDoc();
}

Can we "fix" it by placing it in a try-block? Then exactly what
would we do in the corresponding catch-clause? How many catch-
clauses would we need? What would we do in the presumably
necessary 'catch(...)' clause? Oh, and we have to make sure we
test std::uncaught_exceptions(), too.

OTOH, if we write "pdf_dc_.EndDoc();" in a close() function,
we don't have to ask ourselves these questions.

> But if we don't throw from this
> dtor, the best we can do is what I'm doing now, i.e. show a warning.

[the context here is ~numbered_page()]

Yes, but I worry about using warning() to show such a warning:

    if(extra_pages_ && !std::uncaught_exceptions())
        {
        warning()
            << "Logic error: there should have been "
            << extra_pages_
            << " more page(s) after the page "
            << this_page_number_
            << LMI_FLUSH
            ;

What happens if that series of stream inserters throws?

I think that (as in branch 'odd/dtor-verifies-postcondition')
we should instead write:

    if(extra_pages_ && !std::uncaught_exceptions())
        {
        safely_show_message("Page count is wrong: please report this.");

IOW, we're skating on thin ice when we code a dtor, and any
misstep may be catastrophic. Writing the body of a dtor calls
for almost as much caution as coding a C signal handler. In
a way it's much worse, as seen with the deliberately-throwing
dtor that I tried to write but couldn't: every dtor all the
way up the inheritance chain must be marked "noexcept(false)",
but if we miss marking a single one, then the ice breaks and we
fall through...and gcc won't warn us. It feels like we're
fighting the language, and the compiler isn't on our side.

Of course, if we regard some error detected in a dtor as so
grave as to call for termination, we could just write
    safely_show_message("Something awful has happened.");
    std::exit(EXIT_FAILURE);
in the dtor.