[PATCH 2 of 2 STABLE] transaction: ignore I/O errors during abort logging (issue5658)

[Gregory Szorc]

On Mon, Aug 14, 2017 at 4:40 PM, Jun Wu <quark at fb.com> wrote:

> Thanks for root causing this! There are a few other "self.report" calls.
> Maybe change "report" in __init__? I also suspect if we want to swallow
> other exceptions "report" may raise.
>
>   def __init__(self, report, ...):
>       def safereport(msg):
>           try:
>               report(msg)
>           except Exception:
>               pass
>       self.report = safereport
>

Yes, wrapping report() is better than the current patch. But it still isn't
sufficient because various callbacks can call e.g. ui.write() and run into
the same trouble. I'd really like the transaction to hold a reference to a
ui instance so we can monkeypatch the I/O methods during critical sections.
Not sure why we don't carry a ui reference around. And that change probably
isn't acceptable for stable :/

Also, yes, safereport() should probably trap Exception.

Also, I don't think we want to trap errors in report() globally. The reason
is that we probably want an IOError during e.g. normal transaction
operations to result in an abort. If so, I should add a test for that
behavior...


>
> Excerpts from Gregory Szorc's message of 2017-08-14 13:52:58 -0700:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc at gmail.com>
> > # Date 1502742785 25200
> > #      Mon Aug 14 13:33:05 2017 -0700
> > # Branch stable
> > # Node ID 7e80460cf08e68d812c0e2e662e3b93201cafe4f
> > # Parent  0a33f202bca4ee7ea126e7638bb74b5d58775858
> > transaction: ignore I/O errors during abort logging (issue5658)
> >
> > e9646ff34d55 and 1bfb9a63b98e refactored ui methods to no longer
> > silently swallow some IOError instances.
> >
> > This had the unfortunate side-effect of causing StdioError to
> > bubble up to transaction aborts, leading to an uncaught exception
> > and failure to roll back a transaction. This could occur when a
> > remote HTTP or SSH client connection dropped (possibly via ^C).
> >
> > This commit adds code in transaction abort to explicitly ignore
> > StdioError in some cases.
> >
> > The solution here is extremely brittle and not at all complete.
> > For example, if an abort callback handler performs a ui.write()
> > and gets a StdioError, we still have an incomplete rollback. We
> > can't ignore StdioError from transaction._abort() because we have
> > no clue in what context it was raised and if the abort callback
> > did its job.
> >
> > The proper solution is to make some stdio I/O errors non-fatal
> > during transaction abort. Individual call sites shouldn't have
> > to know to catch and ignore errors during logging, IMO. This was
> > the previous behavior before e9646ff34d55 and 1bfb9a63b98e.
> > I'm not sure how to implement this in a manner appropriate for
> > stable because the abort method and transaction object don't have
> > an explicit handle on the ui instance. We could potentially
> > revert e9646ff34d55 and 1bfb9a63b98e. But I'm not sure of the
> > consequences.
> >
> > diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> > --- a/mercurial/transaction.py
> > +++ b/mercurial/transaction.py
> > @@ -569,9 +569,23 @@ class transaction(object):
> >                      self.opener.unlink(self.journal)
> >                  return
> >
> > -            self.report(_("transaction abort!\n"))
> > +            # stdio here may be connected to a client, which may have
> > +            # disconnected, leading to an I/O error. We treat these as
> > +            # non-fatal because failure to print a logging message
> should
> > +            # not interfere with basic transaction behavior.
> > +            # TODO consider using a context manager or a wrapper around
> the
> > +            # underlying ui that makes I/O errors non-fatal. Because
> trapping
> > +            # each call site is laborious and error prone.
> > +            try:
> > +                self.report(_("transaction abort!\n"))
> > +            except error.StdioError:
> > +                pass
> >
> >              try:
> > +                # We can't reliably catch StdioError here because we
> don't
> > +                # know what the appropriate action to take for any given
> > +                # callback is. So, errors here will result in incomplete
> > +                # rollback until we ignore StdioError at the source.
> >                  for cat in sorted(self._abortcallback):
> >                      self._abortcallback[cat](self)
> >                  # Prevent double usage and help clear cycles.
> > @@ -579,7 +593,11 @@ class transaction(object):
> >                  _playback(self.journal, self.report, self.opener,
> self._vfsmap,
> >                            self.entries, self._backupentries, False,
> >                            checkambigfiles=self.checkambigfiles)
> > -                self.report(_("rollback completed\n"))
> > +
> > +                try:
> > +                    self.report(_("rollback completed\n"))
> > +                except error.StdioError:
> > +                    pass
> >              except BaseException:
> >                  self.report(_("rollback failed - please run hg
> recover\n"))
> >          finally:
> > diff --git a/tests/test-rollback.t b/tests/test-rollback.t
> > --- a/tests/test-rollback.t
> > +++ b/tests/test-rollback.t
> > @@ -263,14 +263,12 @@ An I/O error writing "transaction abort"
> >
> >    $ echo 1 > foo
> >    $ hg --config ui.ioerrors=txnabort --config hooks.pretxncommit=false
> commit -m 'error during abort message'
> > -  *: DeprecationWarning: use lock.release instead of del lock (glob)
> > -    return -1
> > +  warn during abort
> > +  rollback completed
> > +  abort: pretxncommit hook exited with status 1
> >    [255]
> >
> >    $ hg commit -m 'commit 1'
> > -  abort: abandoned transaction found!
> > -  (run 'hg recover' to clean up transaction)
> > -  [255]
> >
> >    $ cd ..
> >
> > @@ -296,7 +294,6 @@ An I/O error writing "rollback completed
> >    $ hg --config ui.ioerrors=txnrollback --config
> hooks.pretxncommit=false commit -m 'error during rollback message'
> >    transaction abort!
> >    warn during abort
> > -  rollback failed - please run hg recover
> >    abort: pretxncommit hook exited with status 1
> >    [255]
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170814/15037949/attachment.html>