Re: [PATCH 4 of 4 chg-tests-fix] commandserver: handle IOError related to flushing of streams

2020-12-08 Thread Yuya Nishihara
On Tue, 08 Dec 2020 12:58:24 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pul...@gmail.com>
> # Date 1606996129 -19800
> #  Thu Dec 03 17:18:49 2020 +0530
> # Node ID acdb053888ab7564b6b7fc702f94334ef854861c
> # Parent  263a5b17b4cad2e793817bd30944dcfcebc88a69
> # EXP-Topic chg-test
> commandserver: handle IOError related to flushing of streams
> 
> After dispatch, without chg we have handling of flushing of streams and
> exception handling related to it. The exception handling part is important
> because there can be exceptions when flushing fout or ferr.
> 
> One such case is in `test-basic.t` which was failing on python3+chg without 
> this
> patch as this handling was missing from chg.
> 
> Failure can be seen at
> https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/128399
> 
> Honestly I am not sure which one of `chgserver.py` or `commandserver.py` the
> change should go in.
> 
> Differential Revision: https://phab.mercurial-scm.org/D9517
> 
> diff -r 263a5b17b4ca -r acdb053888ab mercurial/commandserver.py
> --- a/mercurial/commandserver.py  Wed Dec 02 14:27:45 2020 +0530
> +++ b/mercurial/commandserver.py  Thu Dec 03 17:18:49 2020 +0530
> @@ -355,7 +355,18 @@
>  )
>  
>  try:
> -ret = self._dispatchcommand(req) & 255
> +err = None
> +try:
> +status = self._dispatchcommand(req)
> +except error.StdioError as e:
> +status = -1
> +err = e
> +
> +retval = dispatch.closestdio(req.ui, err)

Catching StdioError here seems fine, but the function name is misleading.
stdio should never be closed here.

> --- a/mercurial/dispatch.py   Wed Dec 02 14:27:45 2020 +0530
> +++ b/mercurial/dispatch.py   Thu Dec 03 17:18:49 2020 +0530
> @@ -104,6 +104,35 @@
>  raise exc
>  
>  
> +def closestdio(ui, err):
> +status = None
> +# In all cases we try to flush stdio streams.
> +if util.safehasattr(ui, b'fout'):
> +assert ui is not None  # help pytype
> +assert ui.fout is not None  # help pytype
> +try:
> +ui.fout.flush()
> +except IOError as e:
> +err = e
> +status = -1
> +
> +if util.safehasattr(ui, b'ferr'):
> +assert ui is not None  # help pytype
> +assert ui.ferr is not None  # help pytype
> +try:
> +if err is not None and err.errno != errno.EPIPE:
> +ui.ferr.write(
> +b'abort: %s\n' % encoding.strtolocal(err.strerror)
> +)
> +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
> +
> +return status

Maybe we can instead move this to dispatch.dispatch() since both hg cli and
commandserver need to translate StdioError to status -1.

def dispatch(...):
try:
... original dispatch()
except StdioError ...:

.. do flushing stuff ...
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


[PATCH 4 of 4 chg-tests-fix] commandserver: handle IOError related to flushing of streams

2020-12-07 Thread Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <7895pul...@gmail.com>
# Date 1606996129 -19800
#  Thu Dec 03 17:18:49 2020 +0530
# Node ID acdb053888ab7564b6b7fc702f94334ef854861c
# Parent  263a5b17b4cad2e793817bd30944dcfcebc88a69
# EXP-Topic chg-test
commandserver: handle IOError related to flushing of streams

After dispatch, without chg we have handling of flushing of streams and
exception handling related to it. The exception handling part is important
because there can be exceptions when flushing fout or ferr.

One such case is in `test-basic.t` which was failing on python3+chg without this
patch as this handling was missing from chg.

Failure can be seen at
https://foss.heptapod.net/octobus/mercurial-devel/-/jobs/128399

Honestly I am not sure which one of `chgserver.py` or `commandserver.py` the
change should go in.

Differential Revision: https://phab.mercurial-scm.org/D9517

diff -r 263a5b17b4ca -r acdb053888ab mercurial/commandserver.py
--- a/mercurial/commandserver.pyWed Dec 02 14:27:45 2020 +0530
+++ b/mercurial/commandserver.pyThu Dec 03 17:18:49 2020 +0530
@@ -355,7 +355,18 @@
 )
 
 try:
-ret = self._dispatchcommand(req) & 255
+err = None
+try:
+status = self._dispatchcommand(req)
+except error.StdioError as e:
+status = -1
+err = e
+
+retval = dispatch.closestdio(req.ui, err)
+if retval:
+status = retval
+
+ret = status & 255
 # If shutdown-on-interrupt is off, it's important to write the
 # result code *after* SIGINT handler removed. If the result code
 # were lost, the client wouldn't be able to continue processing.
diff -r 263a5b17b4ca -r acdb053888ab mercurial/dispatch.py
--- a/mercurial/dispatch.py Wed Dec 02 14:27:45 2020 +0530
+++ b/mercurial/dispatch.py Thu Dec 03 17:18:49 2020 +0530
@@ -104,6 +104,35 @@
 raise exc
 
 
+def closestdio(ui, err):
+status = None
+# In all cases we try to flush stdio streams.
+if util.safehasattr(ui, b'fout'):
+assert ui is not None  # help pytype
+assert ui.fout is not None  # help pytype
+try:
+ui.fout.flush()
+except IOError as e:
+err = e
+status = -1
+
+if util.safehasattr(ui, b'ferr'):
+assert ui is not None  # help pytype
+assert ui.ferr is not None  # help pytype
+try:
+if err is not None and err.errno != errno.EPIPE:
+ui.ferr.write(
+b'abort: %s\n' % encoding.strtolocal(err.strerror)
+)
+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
+
+return status
+
+
 def run():
 """run the command in sys.argv"""
 try:
@@ -117,30 +146,9 @@
 err = e
 status = -1
 
-# In all cases we try to flush stdio streams.
-if util.safehasattr(req.ui, b'fout'):
-assert req.ui is not None  # help pytype
-assert req.ui.fout is not None  # help pytype
-try:
-req.ui.fout.flush()
-except IOError as e:
-err = e
-status = -1
-
-if util.safehasattr(req.ui, b'ferr'):
-assert req.ui is not None  # help pytype
-assert req.ui.ferr is not None  # help pytype
-try:
-if err is not None and err.errno != errno.EPIPE:
-req.ui.ferr.write(
-b'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
-
+ret = closestdio(req.ui, err)
+if ret:
+status = ret
 _silencestdio()
 except KeyboardInterrupt:
 # Catch early/late KeyboardInterrupt as last ditch. Here nothing will
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel