Patchwork chgserver: backport py3 buffered I/O workarounds from procutil

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 17, 2020, 1:04 p.m.
Message ID <0748354881c942727aef.1605618293@lemosa>
Download mbox | patch
Permalink /patch/47612/
State Accepted
Headers show

Comments

Yuya Nishihara - Nov. 17, 2020, 1:04 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1605608948 -32400
#      Tue Nov 17 19:29:08 2020 +0900
# Node ID 0748354881c942727aef46945d721bc51d213e9d
# Parent  d68618954adef9c2cbec868c1af0e01f67288cb8
chgserver: backport py3 buffered I/O workarounds from procutil

I've recently switched to new machine and I found chg's stdout is fully
buffered.

Even though chg server is a daemon process, it inherits the environment
where the chg client originally forked the server. This means the server's
stdout might have been wrapped by LineBufferedWrapper. That's why we need
to do wrap/unwrap in both ways.

The "if" condition in _restoreio() looks weird, but I'm not willing to
clean things up because stdio behavior is fundamentally different between
py2 and py3, and py2 support will be dropped anyway.
Augie Fackler - Nov. 17, 2020, 6:45 p.m.
queued, thanks

> On Nov 17, 2020, at 08:04, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1605608948 -32400
> #      Tue Nov 17 19:29:08 2020 +0900
> # Node ID 0748354881c942727aef46945d721bc51d213e9d
> # Parent  d68618954adef9c2cbec868c1af0e01f67288cb8
> chgserver: backport py3 buffered I/O workarounds from procutil
> 
> I've recently switched to new machine and I found chg's stdout is fully
> buffered.
> 
> Even though chg server is a daemon process, it inherits the environment
> where the chg client originally forked the server. This means the server's
> stdout might have been wrapped by LineBufferedWrapper. That's why we need
> to do wrap/unwrap in both ways.
> 
> The "if" condition in _restoreio() looks weird, but I'm not willing to
> clean things up because stdio behavior is fundamentally different between
> py2 and py3, and py2 support will be dropped anyway.
> 
> diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> --- a/mercurial/chgserver.py
> +++ b/mercurial/chgserver.py
> @@ -409,14 +409,23 @@ class chgcmdserver(commandserver.server)
>             # be unbuffered no matter if it is a tty or not.
>             if fn == b'ferr':
>                 newfp = fp
> +            elif pycompat.ispy3:
> +                # On Python 3, the standard library doesn't offer line-buffered
> +                # binary streams, so wrap/unwrap it.
> +                if fp.isatty():
> +                    newfp = procutil.make_line_buffered(fp)
> +                else:
> +                    newfp = procutil.unwrap_line_buffered(fp)
>             else:
> -                # make it line buffered explicitly because the default is
> -                # decided on first write(), where fout could be a pager.
> +                # Python 2 uses the I/O streams provided by the C library, so
> +                # make it line-buffered explicitly. Otherwise the default would
> +                # be decided on first write(), where fout could be a pager.
>                 if fp.isatty():
>                     bufsize = 1  # line buffered
>                 else:
>                     bufsize = -1  # system default
>                 newfp = os.fdopen(fp.fileno(), mode, bufsize)
> +            if newfp is not fp:
>                 setattr(ui, fn, newfp)
>             setattr(self, cn, newfp)
> 
> @@ -440,13 +449,16 @@ class chgcmdserver(commandserver.server)
>         ui = self.ui
>         for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios, _iochannels):
>             newfp = getattr(ui, fn)
> -            # close newfp while it's associated with client; otherwise it
> -            # would be closed when newfp is deleted
> -            if newfp is not fp:
> +            # On Python 2, newfp and fp may be separate file objects associated
> +            # with the same fd, so we must close newfp while it's associated
> +            # with the client. Otherwise the new associated fd would be closed
> +            # when newfp gets deleted. On Python 3, newfp is just a wrapper
> +            # around fp even if newfp is not fp, so deleting newfp is safe.
> +            if not (pycompat.ispy3 or newfp is fp):
>                 newfp.close()
>             # restore original fd: fp is open again
>             try:
> -                if newfp is fp and 'w' in mode:
> +                if (pycompat.ispy3 or newfp is fp) and 'w' in mode:
>                     # Discard buffered data which couldn't be flushed because
>                     # of EPIPE. The data should belong to the current session
>                     # and should never persist.
> diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
> --- a/mercurial/utils/procutil.py
> +++ b/mercurial/utils/procutil.py
> @@ -80,6 +80,13 @@ def make_line_buffered(stream):
>     return LineBufferedWrapper(stream)
> 
> 
> +def unwrap_line_buffered(stream):
> +    if isinstance(stream, LineBufferedWrapper):
> +        assert not isinstance(stream.orig, LineBufferedWrapper)
> +        return stream.orig
> +    return stream
> +
> +
> class WriteAllWrapper(object):
>     def __init__(self, orig):
>         self.orig = orig
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -409,14 +409,23 @@  class chgcmdserver(commandserver.server)
             # be unbuffered no matter if it is a tty or not.
             if fn == b'ferr':
                 newfp = fp
+            elif pycompat.ispy3:
+                # On Python 3, the standard library doesn't offer line-buffered
+                # binary streams, so wrap/unwrap it.
+                if fp.isatty():
+                    newfp = procutil.make_line_buffered(fp)
+                else:
+                    newfp = procutil.unwrap_line_buffered(fp)
             else:
-                # make it line buffered explicitly because the default is
-                # decided on first write(), where fout could be a pager.
+                # Python 2 uses the I/O streams provided by the C library, so
+                # make it line-buffered explicitly. Otherwise the default would
+                # be decided on first write(), where fout could be a pager.
                 if fp.isatty():
                     bufsize = 1  # line buffered
                 else:
                     bufsize = -1  # system default
                 newfp = os.fdopen(fp.fileno(), mode, bufsize)
+            if newfp is not fp:
                 setattr(ui, fn, newfp)
             setattr(self, cn, newfp)
 
@@ -440,13 +449,16 @@  class chgcmdserver(commandserver.server)
         ui = self.ui
         for (ch, fp, fd), (cn, fn, mode) in zip(self._oldios, _iochannels):
             newfp = getattr(ui, fn)
-            # close newfp while it's associated with client; otherwise it
-            # would be closed when newfp is deleted
-            if newfp is not fp:
+            # On Python 2, newfp and fp may be separate file objects associated
+            # with the same fd, so we must close newfp while it's associated
+            # with the client. Otherwise the new associated fd would be closed
+            # when newfp gets deleted. On Python 3, newfp is just a wrapper
+            # around fp even if newfp is not fp, so deleting newfp is safe.
+            if not (pycompat.ispy3 or newfp is fp):
                 newfp.close()
             # restore original fd: fp is open again
             try:
-                if newfp is fp and 'w' in mode:
+                if (pycompat.ispy3 or newfp is fp) and 'w' in mode:
                     # Discard buffered data which couldn't be flushed because
                     # of EPIPE. The data should belong to the current session
                     # and should never persist.
diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py
+++ b/mercurial/utils/procutil.py
@@ -80,6 +80,13 @@  def make_line_buffered(stream):
     return LineBufferedWrapper(stream)
 
 
+def unwrap_line_buffered(stream):
+    if isinstance(stream, LineBufferedWrapper):
+        assert not isinstance(stream.orig, LineBufferedWrapper)
+        return stream.orig
+    return stream
+
+
 class WriteAllWrapper(object):
     def __init__(self, orig):
         self.orig = orig