Re: [PATCH 2 of 2 STABLE] transaction: ignore I/O errors during abort logging (issue5658)
On Mon, 14 Aug 2017 17:22:53 -0700, Gregory Szorc wrote: > On Mon, Aug 14, 2017 at 4:40 PM, Jun Wu wrote: > 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. [...] > > > 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. I think it's okay to revert these changesets. Perhaps this problem isn't just for the transaction. Maybe we can take IOError of ui.status/warn/note/debug/write_err as non-fatal because they won't carry the data requested by user. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 2 of 2 STABLE] transaction: ignore I/O errors during abort logging (issue5658)
On Mon, Aug 14, 2017 at 4:40 PM, Jun Wu 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
> > # 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
> >
Re: [PATCH 2 of 2 STABLE] transaction: ignore I/O errors during abort logging (issue5658)
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
> # 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 -
[PATCH 2 of 2 STABLE] transaction: ignore I/O errors during abort logging (issue5658)
# HG changeset patch
# User Gregory Szorc
# 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]
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
