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

[Greg Chicares]

On 2018-03-31 16:18, Vadim Zeitlin wrote:
> On Thu, 29 Mar 2018 20:18:56 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-03-25 16:40, Vadim Zeitlin wrote:
> [...]
> GC> >  Alternatively, and more verbosely, but also less magically, we could 
> just
> GC> > stop calling wxPdfDC::EndDoc() from pdf_writer_wx dtor automatically and
> GC> > require the code using this class to call some close() method 
> explicitly.
> GC> 
> GC> Yes, let's do that, please.

I've cherry-picked and pushed PR 78:

>  Done now, please see https://github.com/vadz/lmi/pull/78, but after
> implementing this, I like it significantly less than before because of all
> the additional checks I felt compelled to add. Basically, we lose the
> compile-time invariant ensuring that the dtor of the object is called once
> and once only and, importantly, that the object can't be used after being
> destroyed and replace it with a run-time invariant that save() must be
> called once and once only and that no other methods can be called after it.
> And this just doesn't seem like a good trade-off.

As I see it, "use dtors only for releasing resources" is a best practice,
and contravening a best practice for convenience is an unfavorable tradeoff.

Why worry about calling save() twice in this particular case, when we have
similar functions that we don't worry about? For example:
  void database_impl::save(std::ostream& index_os, std::ostream& data_os)
  void single_cell_document::write(std::ostream& os) const
Is there something special about PDF files in general, or wxPdfDoc in
particular, which calls for extra precautions that other files don't need?

BTW, AFAICT, wxPdfDocument::EndDoc() doesn't just "end" a document--it might
even write the whole thing, starting with the "%PDF-1.3" in the first few
bytes. If that's the case, then this:
  ~pdf_writer_wx() {if(!std::uncaught_exception()) pdf_dc_.EndDoc();}
would actually write the file, and otherwise it wouldn't be written at all.
But that's not crucial to this discussion.

>  As an aside, it's really unfortunate that there is no way to let the
> compiler check that the object is not used after being moved. Apparently
> there was a "destructive move" proposal (N4034), but it seems to have been
> shelved back in 2015 and is not planned to be added in C++2a. Clang-tidy is
> supposed to be able to detect this problem
> (https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html),
> but Debian version doesn't seem to have any checks at all, so I've been
> unable to test this.

I completely agree. It's much like using a free()d C pointer. C++17
[20.5.5.15] says "moved-from objects shall be placed in a valid but
unspecified state": IOW, they're "undead", and I've watched enough
horror movies to develop an abhorrence for the undead.

> GC> > I suspect that you prefer this solution, but I'd still like to check for
> GC> > uncaught_exceptions() in wxPdfDC dtor to assert (or warn?) if close()
> GC> > hadn't been called without an active exception, as this would be a logic
> GC> > error in this case.
> GC> 
> GC> The perceived advantage of implementing the final phase of the task
> GC> in a dtor is that the dtor is certain to be called at the right
> GC> moment.
> 
>  As mentioned above, there is more than this. I.e. this is advantage #1,
> but the advantage #2 is that it's also certain to be called only once,
> while nothing prevents calling save() twice (at compile-time) and the
> advantage #3 is that no other members can be called on the destroyed
> object, which is again not true in the solution with save().

There's something else I don't understand: why use move semantics here?

Is there some object that is actually moved? I don't see it. I mean, I
do see that pages are moved into pages_, but does save() move anything?

Or did you ref-qualify save() only to prevent any other member function
(other than the dtor) from being called after save()? If so...is that
goal really so important, and is there no other way to achieve it?

> GC> But please review branch 'odd/dtor-verifies-postcondition', where
> GC> I believe I've done what you recommend above, for the progress_meter
> GC> class. The dtor notifies us whenever we've followed an execution
> GC> path that doesn't call culminate() as it should (which would be a
> GC> logic error).
> 
>  Yes, my code is very similar to this branch, but it does have more checks
> because I think it's better to add all the boilerplate rather than risk
> wasting time debugging weird bugs in the future due to not respecting the
> invariants these checks verify.

Are there other checks that I should add to progress_meter? (Actually,
now I've deleted that branch, after moving its contents into production
as commit f5cd9eb84, but further changes can of course be made.)

> GC> >  To summarize, my preferred solution would be to continue calling 
> EndDoc()
> GC> > from the dtor automatically, but skip doing it during stack unwinding. 
> But
> GC> > if you prefer the other one, as I suspect, I could live with it as well.
> GC> 
> GC> Yes, I do strongly "prefer the other one" as you suspected.
> 
>  I don't really expect you to change your opinion even in spite of the
> arguments above, but to me it now seems clear that saving the PDF document
> from pdf_writer_wx dtor is a better solution.

It has advantages iff you're willing to perform actual work in a dtor,
instead of using it only for destruction.

> GC> Consider what happens here if EndDoc() throws:
> GC> 
> GC> pdf_writer_wx::~pdf_writer_wx()
> GC> {
> GC>     // This will finally generate the PDF file.
> GC>     pdf_dc_.EndDoc();
> GC> }
> 
>  This is completely unrelated to the current discussion, but there is
> actually a big problem here, which is that EndDoc() doesn't throw (because
> wxPdfDocument library doesn't use exceptions at all)

I hadn't realized that, but I took a look at the sources, and...hmmm.
Exceptions might still be thrown by std:: facilities it uses, but...
it hardly uses the standard library. Well, I imagine that exceptions
can still arise in wxPdfDoc code through wxWidgets code that it calls.
But it is interesting that it doesn't throw any exceptions itself...
or use return codes either:

> and, worse, it doesn't
> even return anything to allow us to check for its success. So we actually
> should use GetPdfDocument()->SaveAsFile() here, which does return bool, but
> first I need to submit a patch to wxPdfDocument to fix half-missing error
> checking in SaveAsFile() too...
> 
> GC> Can we "fix" it by placing it in a try-block?
> 
>  It would be wrong to "fix" it in this way. If saving the document could
> throw, the exception would need to be propagated because, quite clearly, we
> can't meaningfully handle it in pdf_writer_wx dtor itself. So the correct
> fix would be to make this dtor noexcept(false).

That's the problem with performing work in a dtor. If an exception is
thrown, you probably can't handle it, and you can't rethrow it either.

> GC> IOW, we're skating on thin ice when we code a dtor, and any
> GC> misstep may be catastrophic. Writing the body of a dtor calls
> GC> for almost as much caution as coding a C signal handler. In
> GC> a way it's much worse, as seen with the deliberately-throwing
> GC> dtor that I tried to write but couldn't: every dtor all the
> GC> way up the inheritance chain must be marked "noexcept(false)",
> 
>  AFAICS this is incorrect. Only the base class dtor needs to be marked in
> this way.

Oh. I thought that was the solution to the problem I encountered
when I tried to code that demonstration. But throwing "safely"
from a dtor is not an art I think I should try to master.

>  Actually, thinking back to the beginning of this discussion, I think that
> the matters could be confused by simultaneously discussing two quite
> different issues here. The first of them, which started this discussion,
> was throwing an exception if a page number mismatch was detected in
> numbered_page, instead of just giving a warning() in this class dtor. This
> issue is still _not_ addressed by the PR above and I'll make a separate one
> for it

Okay.

>  However then we've somehow decided to also do the same thing for
> pdf_writer_wx, where the problem is completely different: it's not about
> exceptions being thrown from the dtor, but rather about EndDoc() being
> called from the dtor even during stack unwinding, when it shouldn't be. And
> this problem is, IMO, solved in a much simpler and less error-prone way by
> just adding std::uncaught_exceptions() check as shown above, than by doing
> everything I did in the PR 78.

It comes down to whether or not we believe that dtors should be used
only for releasing resources. I believe that, so I see PR 78 as a big
improvement.

Instead, we could regard a dtor as a combination of two functions:

  C::~C() noexcept(false)
  {
    if(!std::uncaught_exceptions())
      { finalize(); }
    destroy();
  }

Then the usual advice
 - never allow an exception to escape from a dtor
might arguably be refined:
 - never allow an exception to escape from destroy()
 - but feel free to throw in finalize() [*arguably*]

Of course, that's not quite right: the Committee had a reason for
replacing
  bool std::uncaught_exception() // deprecated by C++17
with
  int std::uncaught_exceptions() // C++17
but the code above in effect uses the deprecated function. Yet it
was generally thought that the deprecated function wasn't useful:

http://www.gotw.ca/gotw/047.htm
| My major problem with this solution is not technical, but moral:
| It is poor design to give T::~T() two different semantics ...
| I do not know of any good and safe use for std::uncaught_exception.
| My advice: Don't use it.

and that advice applies equally to
  if(!std::uncaught_exception()) // deprecated
  if(!std::uncaught_exceptions()) // cast to bool
because the cast to bool renders the second equivalent to the first.

I guess it's just ironic that the same author wrote this proposal
for std::uncaught_exceptions():
  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf
which addresses the *technical* objection...while the Committee has
reinforced the *moral* objection by making dtors noexcept(true) by
default.

Now, of course, I do understand that we could

  C::C() : exception_count_(std::uncaught_exceptions())
  {int const exception_count_;}

  C::~C() noexcept(false)
  {
    if(exception_count_ != std::uncaught_exceptions())
      { finalize(); }
    destroy();
  }

(and I agree with your reasoning that it

    // seems highly unlikely that a pdf_writer_wx object would ever be created
    // in a dtor of some other object, which is the only situation in which the
    // two versions would behave differently

in our concrete case), but--in the general case--how does that
address the two objections?

- It resolves the *technical* objection. It makes ScopeGuard work.
(I'm just assuming that, arguendo; I haven't studied it enough to be
certain myself.) BTW, as a corollary:
  ~pdf_writer_wx() {if(!std::uncaught_exception()) pdf_dc_.EndDoc();}
would have been as good a solution in C++98 days as it is now, even
though everyone was against it then.

- It ignores the *moral* objection. C++ chooses not to provide a
"finally" construct:
  http://www.stroustrup.com/bs_faq2.html#finally
Now std::uncaught_exceptions() lets us emulate it, more or less, in
a byzantine way, and it might behave as we hope if we're extremely
careful. But when I tried, I got std::terminate(). I thought that was
because I didn't write "noexcept(false)" in all the right places,
but I guess I was wrong, and I just don't understand it at all.
>From my POV, that constitutes a separate technical objection.

Or maybe I'm conflating the idea of doing actual processing in a dtor
with "finally". If C++ had such a feature, would you write:
  try {write a PDF page}
  catch {handle some possible exception}
  finally {pdf_dc_.EndDoc();}
? And maybe I'm confusing your suggestion with Alexandrescu's
ScopeGuard, which AFAICT is the reason why std::uncaught_exceptions()
was added in C++17; but you wouldn't want to use ScopeGuard here,
would you?

>  Having said all this, I now wonder if I was over-pessimistic in my
> expectations expressed above and if you could, after all, be persuaded to
> change your mind and to use the simpler approach for pdf_writer_wx dtor?

No, the more I think about this, the more I dislike that idea.
C++ dtors are intended for cleanup. ScopeGuard stretches that a
bit to use them for rollback: a niche use case. Using a dtor in
class progress_meter to log a message if an invariant has not been
fulfilled is another inoffensive extension. But using a dtor to
perform work required to *establish* a postcondition seems wrong.

We've observed that well-formed but incorrect PDF files are created
when the original dtor was called. Maybe we could patch that up by
bifurcating the ctor into finally() and destroy() subparts as above;
I'm not convinced that makes everything kosher, and I'm still afraid
of what happens if an exception is thrown. But I suspect that problem
may not have occurred if we had spelled out the procedural steps:
  open file
  write cover page
  write first section
  write another section
  close
instead of using a dtor for mainstream processing. Whatever
wxPdfDocument::EndDoc() actually does is unquestionably far more
than simply releasing a resource, e.g. as fclose() does, or
std::basic_ostream::~basic_ostream().