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

[Jun Wu]

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

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