This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9287

worker: raise exception instead of calling sys.exit() with child's code
ClosedPublic

Authored by martinvonz on Nov 9 2020, 2:42 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG8f07f5a9c3de: worker: raise exception instead of calling sys.exit() with child's code
Summary

When a worker process returns an error code, we would call
sys.exit() with that exit code on the main process. The SystemExit
exception would then get caught in scmutil.callcatch(), which would
return that error code. The comment there says "Commands shouldn't
sys.exit directly", which I agree with. This patch changes it so we
raise a specific exception when a worker fails so we can catch
instead. I think that means that SystemExit is now always an
internal error.

(I had earlier thought that this call to sys.exit() was from within
the child process until Matt Harbison made me look again, so thanks
for that!)

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.

Revision Contents
Changeset List

PathPackages
Mmercurial/error.py (7 lines)
Mmercurial/scmutil.py (3 lines)
Mmercurial/worker.py (2 lines)
Mtests/test-worker.t (5 lines)

Diff 23447

mercurial/error.py

def __init__(self, prefix, matches): def __init__(self, prefix, matches):
self.prefix = prefix self.prefix = prefix
self.matches = matches self.matches = matches
super(AmbiguousCommand, self).__init__() super(AmbiguousCommand, self).__init__()
__bytes__ = _tobytes __bytes__ = _tobytes
class WorkerError(Exception):
"""Exception raised when a worker process dies."""
def __init__(self, status_code):
self.status_code = status_code
class InterventionRequired(Hint, Exception): class InterventionRequired(Hint, Exception):
"""Exception raised when a command requires human intervention.""" """Exception raised when a command requires human intervention."""
__bytes__ = _tobytes __bytes__ = _tobytes
class ConflictResolutionRequired(InterventionRequired): class ConflictResolutionRequired(InterventionRequired):
"""Exception raised when a continuable command required merge conflict resolution.""" """Exception raised when a continuable command required merge conflict resolution."""

mercurial/scmutil.py

ui.error(_(b"(%s)\n") % inst.hint) ui.error(_(b"(%s)\n") % inst.hint)
return 1 return 1
except error.WdirUnsupported: except error.WdirUnsupported:
ui.error(_(b"abort: working directory revision cannot be specified\n")) ui.error(_(b"abort: working directory revision cannot be specified\n"))
except error.Abort as inst: except error.Abort as inst:
ui.error(_(b"abort: %s\n") % inst.message) ui.error(_(b"abort: %s\n") % inst.message)
if inst.hint: if inst.hint:
ui.error(_(b"(%s)\n") % inst.hint) ui.error(_(b"(%s)\n") % inst.hint)
except error.WorkerError as inst:
# Don't print a message -- the worker already should have
return inst.status_code
except ImportError as inst: except ImportError as inst:
ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst)) ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst))
m = stringutil.forcebytestr(inst).split()[-1] m = stringutil.forcebytestr(inst).split()[-1]
if m in b"mpatch bdiff".split(): if m in b"mpatch bdiff".split():
ui.error(_(b"(did you forget to compile extensions?)\n")) ui.error(_(b"(did you forget to compile extensions?)\n"))
elif m in b"zlib".split(): elif m in b"zlib".split():
ui.error(_(b"(is your Python install correct?)\n")) ui.error(_(b"(is your Python install correct?)\n"))
except (IOError, OSError) as inst: except (IOError, OSError) as inst:

mercurial/worker.py

except: # re-raises except: # re-raises
killworkers() killworkers()
cleanup() cleanup()
raise raise
status = cleanup() status = cleanup()
if status: if status:
if status < 0: if status < 0:
os.kill(os.getpid(), -status) os.kill(os.getpid(), -status)
sys.exit(status) raise error.WorkerError(status)
if hasretval: if hasretval:
yield True, retval yield True, retval
def _posixexitstatus(code): def _posixexitstatus(code):
'''convert a posix exit status into the same form returned by '''convert a posix exit status into the same form returned by
os.spawnv os.spawnv

tests/test-worker.t

$ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \ $ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \
> test 100000.0 abort 2>&1 > test 100000.0 abort 2>&1
start start
abort: known exception abort: known exception
[255] [255]
$ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \ $ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \
> test 100000.0 abort --traceback 2>&1 | egrep '(SystemExit|Abort)' > test 100000.0 abort --traceback 2>&1 | egrep '(WorkerError|Abort)'
raise error.Abort(b'known exception') raise error.Abort(b'known exception')
mercurial.error.Abort: known exception (py3 !) mercurial.error.Abort: known exception (py3 !)
Abort: known exception (no-py3 !) Abort: known exception (no-py3 !)
SystemExit: 255 raise error.WorkerError(status)
mercurial.error.WorkerError: 255
Traceback must be printed for unknown exceptions Traceback must be printed for unknown exceptions
$ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \ $ hg --config "extensions.t=$abspath" --config 'worker.numcpus=8' \
> test 100000.0 exc 2>&1 | grep '^Exception' > test 100000.0 exc 2>&1 | grep '^Exception'
Exception: unknown exception Exception: unknown exception
Workers should not do cleanups in all cases Workers should not do cleanups in all cases