This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11627

dispatch: ignore failure to flush ui
ClosedPublic

Authored by martinvonz on Oct 11 2021, 7:30 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG2f2107c01dee: dispatch: ignore failure to flush ui
Summary

When the pager dies, we get a SIGPIPE. That causes
error.SignalInterrupt to be raised (from ui._catchterm()`). Any
further writes or flushes will cause further SIGPIPEs and furhter
error.SignalInterrupt. If we write or flush outside of the
try/except that handle KeyboardInterrupt (which
error.SignalInterrupt is a subclass of), then control will escape
from the dispatch module. Let's fix that by ignoring errors from
flushing the ui.

I would have rather fixed this by restoring the stdout and stderr
streams when the pager dies, but it gets complicated because of
multiple ui instances (ui/lui) and different pager setups between
regular hg and chg.

This changes a test in test-pager.t, but I don't understand why. I
would have thought that all the output from the command should have
gone to the broken pager.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Oct 11 2021, 7:30 PM
pulkit accepted this revision.Oct 12 2021, 9:21 AM
This revision is now accepted and ready to land.Oct 12 2021, 9:21 AM
martinvonz added a commit: rHG2f2107c01dee: dispatch: ignore failure to flush ui.Oct 12 2021, 1:20 PM
Closed by commit rHG2f2107c01dee: dispatch: ignore failure to flush ui (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
Alphare added a subscriber: Alphare.Oct 18 2021, 5:29 AM

This breaks CI on Python 2: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254041

We unfortunately have to support Python 2 until the release in a few days, so this should get fixed.

martinvonz added a comment.Oct 18 2021, 12:02 PM
In D11627#179096, @Alphare wrote:

This breaks CI on Python 2: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254041
We unfortunately have to support Python 2 until the release in a few days, so this should get fixed.

It doesn't reproduce for me. Does it for you? I wonder what I need to do to make it reproduce. I have /dev/full.

Alphare added a comment.Oct 18 2021, 12:11 PM
In D11627#179220, @martinvonz wrote:
In D11627#179096, @Alphare wrote:

This breaks CI on Python 2: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254041
We unfortunately have to support Python 2 until the release in a few days, so this should get fixed.

It doesn't reproduce for me. Does it for you? I wonder what I need to do to make it reproduce. I have /dev/full.

It does, I'm on Linux if somehow this is OS specific.

$ HGMODULEPOLICY=py python2 ./tests/run-tests.py -j6 tests/test-basic.t
running 1 tests using 1 parallel processes

--- /home/alphare/workspace/octobus/mercurial-devel/tests/test-basic.t
+++ /home/alphare/workspace/octobus/mercurial-devel/tests/test-basic.t.err
@@ -40,13 +40,10 @@
   A a

   $ hg status >/dev/full
-  abort: No space left on device
-  [255]
 #endif

 #if devfull
   $ hg status >/dev/full 2>&1
-  [255]

   $ hg status ENOENT 2>/dev/full
   [255]

ERROR: test-basic.t output changed
!
Failed test-basic.t: output changed
# Ran 1 tests, 0 skipped, 1 failed.
python hash seed: 454035856
martinvonz added a comment.Oct 18 2021, 12:17 PM
In D11627#179224, @Alphare wrote:
In D11627#179220, @martinvonz wrote:
In D11627#179096, @Alphare wrote:

This breaks CI on Python 2: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254041
We unfortunately have to support Python 2 until the release in a few days, so this should get fixed.

It doesn't reproduce for me. Does it for you? I wonder what I need to do to make it reproduce. I have /dev/full.

It does, I'm on Linux if somehow this is OS specific.

$ HGMODULEPOLICY=py python2 ./tests/run-tests.py -j6 tests/test-basic.t
running 1 tests using 1 parallel processes
--- /home/alphare/workspace/octobus/mercurial-devel/tests/test-basic.t
+++ /home/alphare/workspace/octobus/mercurial-devel/tests/test-basic.t.err
@@ -40,13 +40,10 @@
   A a
   $ hg status >/dev/full
-  abort: No space left on device
-  [255]
 #endif
 #if devfull
   $ hg status >/dev/full 2>&1
-  [255]
   $ hg status ENOENT 2>/dev/full
   [255]
ERROR: test-basic.t output changed
!
Failed test-basic.t: output changed
# Ran 1 tests, 0 skipped, 1 failed.
python hash seed: 454035856

Oh, it does reproduce for me too now. Sorry, I had forgotten that I updated to another commit while trying to figure out how to run the python2 test. I'll see if I can figure out what's happening there. Worst case we roll it back and wait until after we don't care about py2 anymore.

Alphare added a comment.Oct 18 2021, 12:34 PM
In D11627#179225, @martinvonz wrote:

Oh, it does reproduce for me too now. Sorry, I had forgotten that I updated to another commit while trying to figure out how to run the python2 test. I'll see if I can figure out what's happening there. Worst case we roll it back and wait until after we don't care about py2 anymore.

Thanks! That option also works for me. :)

Revision Contents
Changeset List

PathPackages
Mmercurial/dispatch.py (26 lines)
Mtests/test-pager.t (8 lines)

Diff 30746

mercurial/dispatch.py

"""run the command specified in req.args; returns an integer status code""" """run the command specified in req.args; returns an integer status code"""
err = None err = None
try: try:
status = _rundispatch(req) status = _rundispatch(req)
except error.StdioError as e: except error.StdioError as e:
err = e err = e
status = -1 status = -1
# Somehow we have to catcht he exception here; catching it inside
# _flushstdio() doesn't work.
try:
ret = _flushstdio(req.ui, err) ret = _flushstdio(req.ui, err)
if ret and not status: if ret and not status:
status = ret status = ret
except BaseException:
pass
return status return status
def _rundispatch(req): def _rundispatch(req):
with tracing.log('dispatch._rundispatch'): with tracing.log('dispatch._rundispatch'):
if req.ferr: if req.ferr:
ferr = req.ferr ferr = req.ferr
elif req.ui: elif req.ui:
# SIGPIPE was raised. we cannot print anything in this case. # SIGPIPE was raised. we cannot print anything in this case.
pass pass
except IOError as inst: except IOError as inst:
if inst.errno != errno.EPIPE: if inst.errno != errno.EPIPE:
raise raise
ret = -1 ret = -1
finally: finally:
duration = util.timer() - starttime duration = util.timer() - starttime
try:
req.ui.flush() # record blocked times req.ui.flush() # record blocked times
except BaseException:
pass
if req.ui.logblockedtimes: if req.ui.logblockedtimes:
req.ui._blockedtimes[b'command_duration'] = duration * 1000 req.ui._blockedtimes[b'command_duration'] = duration * 1000
req.ui.log( req.ui.log(
b'uiblocked', b'uiblocked',
b'ui blocked ms\n', b'ui blocked ms\n',
**pycompat.strkwargs(req.ui._blockedtimes) **pycompat.strkwargs(req.ui._blockedtimes)
) )
return_code = ret & 255 return_code = ret & 255
req.ui.log( req.ui.log(
b"commandfinish", b"commandfinish",
b"%s exited %d after %0.2f seconds\n", b"%s exited %d after %0.2f seconds\n",
msg, msg,
return_code, return_code,
duration, duration,
return_code=return_code, return_code=return_code,
duration=duration, duration=duration,
canonical_command=req.canonical_command, canonical_command=req.canonical_command,
) )
try: try:
req._runexithandlers() req._runexithandlers()
except: # exiting, so no re-raises except: # exiting, so no re-raises
ret = ret or -1 ret = ret or -1
# do flush again since ui.log() and exit handlers may write to ui # do flush again since ui.log() and exit handlers may write to ui
try:
req.ui.flush() req.ui.flush()
except BaseException:
pass
return ret return ret
def _runcatch(req): def _runcatch(req):
with tracing.log('dispatch._runcatch'): with tracing.log('dispatch._runcatch'):
def catchterm(*args): def catchterm(*args):
raise error.SignalInterrupt raise error.SignalInterrupt
) )
% debugger % debugger
) )
with demandimport.deactivated(): with demandimport.deactivated():
debugtrace[debugger]() debugtrace[debugger]()
try: try:
return _dispatch(req) return _dispatch(req)
finally: finally:
ui.flush() try:
ui.flush() # record blocked times
except BaseException:
pass
except: # re-raises except: # re-raises
# enter the debugger when we hit an exception # enter the debugger when we hit an exception
if req.earlyoptions[b'debugger']: if req.earlyoptions[b'debugger']:
traceback.print_exc() traceback.print_exc()
debugmortem[debugger](sys.exc_info()[2]) debugmortem[debugger](sys.exc_info()[2])
raise raise
return _callcatch(ui, _runcatchfunc) return _callcatch(ui, _runcatchfunc)

tests/test-pager.t

\x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc) \x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc)
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: modify a 8 summary: modify a 8
#endif #endif
A complicated pager command gets worse behavior. Bonus points if you can A complicated pager command gets worse behavior. Bonus points if you can
improve this. Windows apparently does this better, but only sometimes? improve this.
#if windows
$ hg log --limit 3 \ $ hg log --limit 3 \
> --config pager.pager='this-command-better-never-exist --seriously' \ > --config pager.pager='this-command-better-never-exist --seriously' \
> 2>/dev/null || true > 2>/dev/null || true
\x1b[0;33mchangeset: 10:46106edeeb38\x1b[0m (esc) (?) \x1b[0;33mchangeset: 10:46106edeeb38\x1b[0m (esc) (?)
tag: tip (?) tag: tip (?)
user: test (?) user: test (?)
date: Thu Jan 01 00:00:00 1970 +0000 (?) date: Thu Jan 01 00:00:00 1970 +0000 (?)
summary: modify a 10 (?) summary: modify a 10 (?)
(?) (?)
\x1b[0;33mchangeset: 9:6dd8ea7dd621\x1b[0m (esc) (?) \x1b[0;33mchangeset: 9:6dd8ea7dd621\x1b[0m (esc) (?)
user: test (?) user: test (?)
date: Thu Jan 01 00:00:00 1970 +0000 (?) date: Thu Jan 01 00:00:00 1970 +0000 (?)
summary: modify a 9 (?) summary: modify a 9 (?)
(?) (?)
\x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc) (?) \x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc) (?)
user: test (?) user: test (?)
date: Thu Jan 01 00:00:00 1970 +0000 (?) date: Thu Jan 01 00:00:00 1970 +0000 (?)
summary: modify a 8 (?) summary: modify a 8 (?)
(?) (?)
#else
$ hg log --limit 3 \
> --config pager.pager='this-command-better-never-exist --seriously' \
> 2>/dev/null || true
#endif
Pager works with shell aliases. Pager works with shell aliases.
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [alias] > [alias]
> echoa = !echo a > echoa = !echo a
> EOF > EOF