Patchwork D3445: dispatch: shore up exit handling

login
register
mail settings
Submitter phabricator
Date May 6, 2018, 4:11 a.m.
Message ID <differential-rev-PHID-DREV-b7wpuvx3qdiqh3za5sqb-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31279/
State New
Headers show

Comments

phabricator - May 6, 2018, 4:11 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This commit overhauls exit handling to be more robust and agree
  with behavior of CPython.
  
  If a non-None, non-int value is seen, we print it and set the exit
  code to 1, just like what Python does.
  
  We also catch SystemExit explicitly and coerce it to an integer
  exit code. This is essentially what CPython does.
  
  As part of the code refactor, we also now explicitly handle an IOError
  when flushing stderr.
  
  As the tests demonstrate, various crashes disappear.
  
  There's potentially some more testing to be done around IOError in
  this function. But things should be no worse than they formerly were.
  
  The Python 3 test failures introduced previously have all gone away.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3445

AFFECTED FILES
  mercurial/dispatch.py
  tests/test-dispatch.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - May 6, 2018, 12:18 p.m.
IIRC, SystemExit is caught at callcatch and translated to a status code.

> +    # The logic here attempts to mimic how CPython's pythonrun.c does things.
> +    # Essentially:
> +    #
> +    # * Exit is controlled by returning a value or raising SystemExit.
> +    # * An integer value exits with that exit code.
> +    # * A value of ``None`` is interpreted as ``0``.
> +    # * A non-integer, non-None value results in that value being printed
> +    #   and an exit code of ``1``.
> +    # * SystemExit can have a ``code`` attribute containing the exit value.
>      try:
> -        status = (dispatch(req) or 0)
> +        code = dispatch(req) or 0
>      except error.StdioError as e:
>          err = e
> -        status = -1
> +        code = -1
> +    except SystemExit as err:
> +        try:
> +            code = err.code
> +            err = None
> +        except AttributeError:
phabricator - May 6, 2018, 12:29 p.m.
yuja added a comment.


  IIRC, SystemExit is caught at callcatch and translated to a status code.
  
  > +    # The logic here attempts to mimic how CPython's pythonrun.c does things.
  >  +    # Essentially:
  >  +    #
  >  +    # * Exit is controlled by returning a value or raising SystemExit.
  >  +    # * An integer value exits with that exit code.
  >  +    # * A value of ``None`` is interpreted as ``0``.
  >  +    # * A non-integer, non-None value results in that value being printed
  >  +    #   and an exit code of ``1``.
  >  +    # * SystemExit can have a ``code`` attribute containing the exit value.
  > 
  >   try:
  > 
  > - status = (dispatch(req) or 0) +        code = dispatch(req) or 0 except error.StdioError as e: err = e
  > - status = -1 +        code = -1 +    except SystemExit as err: +        try: +            code = err.code +            err = None +        except AttributeError:

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3445

To: indygreg, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
--- a/tests/test-dispatch.t
+++ b/tests/test-dispatch.t
@@ -263,48 +263,28 @@ 
   [42]
 
   $ hg exit returnstring 'some message'
-  Traceback (most recent call last):
-    File "*/hg", line *, in <module> (glob)
-      dispatch.run()
-    File "*/mercurial/dispatch.py", line *, in run (glob)
-      sys.exit(status & 255)
-  TypeError: unsupported operand type(s) for &: 'str' and 'int'
+  'some message'
   [1]
 
   $ hg exit returnnone
 
   $ hg exit returnempty
 
   $ hg exit returndict
-  Traceback (most recent call last):
-    File "*/hg", line *, in <module> (glob)
-      dispatch.run()
-    File "*/mercurial/dispatch.py", line *, in run (glob)
-      sys.exit(status & 255)
-  TypeError: unsupported operand type(s) for &: 'dict' and 'int'
+  {'key1': 'value1'}
   [1]
 
   $ hg exit systemexitint 42
   [42]
 
   $ hg exit systemexitstring 'failure message'
-  Traceback (most recent call last):
-    File "*/hg", line *, in <module> (glob)
-      dispatch.run()
-    File "*/mercurial/dispatch.py", line *, in run (glob)
-      sys.exit(status & 255)
-  TypeError: unsupported operand type(s) for &: 'str' and 'int'
+  'failure message'
   [1]
 
   $ hg exit systemexitnoarg
 
   $ hg exit systemexitnone
 
   $ hg exit systemexitdict
-  Traceback (most recent call last):
-    File "*/hg", line *, in <module> (glob)
-      dispatch.run()
-    File "*/mercurial/dispatch.py", line *, in run (glob)
-      sys.exit(status & 255)
-  TypeError: unsupported operand type(s) for &: 'dict' and 'int'
+  {'key1': 'value1'}
   [1]
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -86,30 +86,66 @@ 
     _initstdio()
     req = request(pycompat.sysargv[1:])
     err = None
+
+    # The logic here attempts to mimic how CPython's pythonrun.c does things.
+    # Essentially:
+    #
+    # * Exit is controlled by returning a value or raising SystemExit.
+    # * An integer value exits with that exit code.
+    # * A value of ``None`` is interpreted as ``0``.
+    # * A non-integer, non-None value results in that value being printed
+    #   and an exit code of ``1``.
+    # * SystemExit can have a ``code`` attribute containing the exit value.
     try:
-        status = (dispatch(req) or 0)
+        code = dispatch(req) or 0
     except error.StdioError as e:
         err = e
-        status = -1
+        code = -1
+    except SystemExit as err:
+        try:
+            code = err.code
+            err = None
+        except AttributeError:
+            pass
+
+    # In all cases we flush our stdio streams.
     if util.safehasattr(req.ui, 'fout'):
         try:
             req.ui.fout.flush()
         except IOError as e:
             err = e
-            status = -1
+            code = -1
+
     if util.safehasattr(req.ui, 'ferr'):
         try:
             if err is not None and err.errno != errno.EPIPE:
                 req.ui.ferr.write('abort: %s\n' %
                                   encoding.strtolocal(err.strerror))
-            req.ui.ferr.flush()
         # There's not much we can do about an I/O error here. So (possibly)
         # change the status code and move on.
         except IOError:
-            status = -1
+            code = -1
 
-    _silencestdio()
-    sys.exit(status & 255)
+    # If we got a non-integer exit code, we print that as well.
+    if not isinstance(code, int) and util.safehasattr(req.ui, 'ferr'):
+        try:
+            req.ui.ferr.write('%r\n' % code)
+        # This will ignore errors formatting the value and errors printing
+        # to stderr. That's intended. At this point, there's not much
+        # we can do about any errors.
+        except Exception:
+            pass
+
+        code = 1
+
+    if util.safehasattr(req.ui, 'ferr'):
+        try:
+            req.ui.ferr.flush()
+        except IOError:
+            # There's not much we can do about I/O errors at this point.
+            code = -1
+
+    sys.exit(code & 255)
 
 if pycompat.ispy3:
     def _initstdio():