Patchwork [5,of,9] stdio: catch StdioError in dispatch.run and clean up appropriately

login
register
mail settings
Submitter Bryan O'Sullivan
Date April 10, 2017, 6:51 p.m.
Message ID <024364c1aa2e8b247d61.1491850295@bryano-mbp.local>
Download mbox | patch
Permalink /patch/20090/
State Changes Requested
Headers show

Comments

Bryan O'Sullivan - April 10, 2017, 6:51 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1490804813 25200
#      Wed Mar 29 09:26:53 2017 -0700
# Node ID 024364c1aa2e8b247d6156208394ebf01913f85a
# Parent  935d0e5af9001f3a0730e7c477dc14ae5baf4799
stdio: catch StdioError in dispatch.run and clean up appropriately

We attempt to report what went wrong, and more importantly exit the
program with an error code.

(The exception we catch is not yet raised anywhere in the code.)
Yuya Nishihara - April 11, 2017, 1:54 p.m.
On Mon, 10 Apr 2017 11:51:35 -0700, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bryano@fb.com>
> # Date 1490804813 25200
> #      Wed Mar 29 09:26:53 2017 -0700
> # Node ID 024364c1aa2e8b247d6156208394ebf01913f85a
> # Parent  935d0e5af9001f3a0730e7c477dc14ae5baf4799
> stdio: catch StdioError in dispatch.run and clean up appropriately
> 
> We attempt to report what went wrong, and more importantly exit the
> program with an error code.
> 
> (The exception we catch is not yet raised anywhere in the code.)
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -60,7 +60,22 @@ class request(object):
>  
>  def run():
>      "run the command in sys.argv"
> -    sys.exit((dispatch(request(pycompat.sysargv[1:])) or 0) & 255)
> +    req = request(pycompat.sysargv[1:])
> +    err = None
> +    try:
> +        status = (dispatch(req) or 0) & 255
> +    except error.StdioError as err:
> +        status = 1
> +    if util.safehasattr(req.ui, 'fout'):
> +        try:
> +            req.ui.fout.close()
> +        except IOError as err:
> +            status = 1

255 would be better, too.

> +    if util.safehasattr(req.ui, 'ferr'):
> +        if err is not None and err.errno != errno.EPIPE:
> +            req.ui.ferr.write('abort: %s\n' % err.strerror)
> +        req.ui.ferr.close()
> +    sys.exit(status)

Is it safe to close stdout and stderr here? IIRC, Python interpreter can
somehow crash due to closed stdout/err on Windows.
Bryan O'Sullivan - April 11, 2017, 5:54 p.m.
On Tue, Apr 11, 2017 at 6:54 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> 255 would be better, too.
>

Done.


> > +    if util.safehasattr(req.ui, 'ferr'):
> > +        if err is not None and err.errno != errno.EPIPE:
> > +            req.ui.ferr.write('abort: %s\n' % err.strerror)
> > +        req.ui.ferr.close()
> > +    sys.exit(status)
>
> Is it safe to close stdout and stderr here? IIRC, Python interpreter can
> somehow crash due to closed stdout/err on Windows.
>

The interpreter will crash if the file descriptor backing a file handle is
closed, but closing the file handle itself isn't a problem.
Yuya Nishihara - April 12, 2017, 1:31 p.m.
On Tue, 11 Apr 2017 10:54:00 -0700, Bryan O'Sullivan wrote:
> On Tue, Apr 11, 2017 at 6:54 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > +    if util.safehasattr(req.ui, 'ferr'):
> > > +        if err is not None and err.errno != errno.EPIPE:
> > > +            req.ui.ferr.write('abort: %s\n' % err.strerror)
> > > +        req.ui.ferr.close()
> > > +    sys.exit(status)
> >
> > Is it safe to close stdout and stderr here? IIRC, Python interpreter can
> > somehow crash due to closed stdout/err on Windows.
> 
> The interpreter will crash if the file descriptor backing a file handle is
> closed, but closing the file handle itself isn't a problem.

Thanks for the clarification. We have two file objects attached to the same
stdout fd (see 3a4c0905f357), so fout.close() will be problem. Perhaps, we
only need to do fout.flush() here?

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -60,7 +60,22 @@  class request(object):
 
 def run():
     "run the command in sys.argv"
-    sys.exit((dispatch(request(pycompat.sysargv[1:])) or 0) & 255)
+    req = request(pycompat.sysargv[1:])
+    err = None
+    try:
+        status = (dispatch(req) or 0) & 255
+    except error.StdioError as err:
+        status = 1
+    if util.safehasattr(req.ui, 'fout'):
+        try:
+            req.ui.fout.close()
+        except IOError as err:
+            status = 1
+    if util.safehasattr(req.ui, 'ferr'):
+        if err is not None and err.errno != errno.EPIPE:
+            req.ui.ferr.write('abort: %s\n' % err.strerror)
+        req.ui.ferr.close()
+    sys.exit(status)
 
 def _getsimilar(symbols, value):
     sim = lambda x: difflib.SequenceMatcher(None, value, x).ratio()