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

[Vadim Zeitlin]

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.

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

 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.


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. 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. But
this is not enough to prevent a wrong PDF from being generated, even with
the change discussed above.

GC> Now we have a situation that calls for rollback semantics, which
GC> can now (2) theoretically be implemented in dtors. But I think we're
GC> also saying that these particular rollback semantics require an
GC> extra step (1),

 This is true right now, but the change to pdf_writer_wx from above would
avoid the need for it, making throwing from numbered_page() much more
appealing and I'd really like to just throw from it.

GC> and in that case the dtor might just as well set a failure flag instead
GC> of throwing.

 We could always use an error flag, but this is more fragile than throwing
an exception, as the flag could be forgotten to be checked, while the
exception would have to be explicitly ignored.


GC> Therefore, it seems that write_ledger_as_pdf() is the right place
GC> to handle PDF-generation errors.

 Thanks for answering this but, finally, if we change pdf_writer_wx and
numbered_page dtors as suggested, we wouldn't need to change anything in
write_ledger_as_pdf() as we can't do anything else than reporting the
exception to the user, which would happen anyhow when it reaches the main
event loop.


 What do you think of the proposed changes?
VZ