Patchwork [STABLE] chgserver: discard buffered output before restoring fds (issue6207)

login
register
mail settings
Submitter Yuya Nishihara
Date July 21, 2020, 12:42 p.m.
Message ID <88f93fc8cb3263a3c101.1595335366@mimosa>
Download mbox | patch
Permalink /patch/46814/
State Accepted
Headers show

Comments

Yuya Nishihara - July 21, 2020, 12:42 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1595244684 -32400
#      Mon Jul 20 20:31:24 2020 +0900
# Branch stable
# Node ID 88f93fc8cb3263a3c10143557cb9626dfcf23f4c
# Parent  f91f0dfccf9b0a928e22bf0041b310712c93903c
chgserver: discard buffered output before restoring fds (issue6207)

On Python 3, flush() appears not discarding buffered data on EPIPE, and
the buffered data will be carried over to the restored stdout.
Augie Fackler - July 21, 2020, 2:39 p.m.
queued for stable, thanks

> On Jul 21, 2020, at 08:42, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1595244684 -32400
> #      Mon Jul 20 20:31:24 2020 +0900
> # Branch stable
> # Node ID 88f93fc8cb3263a3c10143557cb9626dfcf23f4c
> # Parent  f91f0dfccf9b0a928e22bf0041b310712c93903c
> chgserver: discard buffered output before restoring fds (issue6207)
> 
> On Python 3, flush() appears not discarding buffered data on EPIPE, and
> the buffered data will be carried over to the restored stdout.
> 
> diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> --- a/mercurial/chgserver.py
> +++ b/mercurial/chgserver.py
> @@ -434,8 +434,11 @@ class chgcmdserver(commandserver.server)
>             self._oldios.append((ch, fp, fd))
> 
>     def _restoreio(self):
> +        if not self._oldios:
> +            return
> +        nullfd = os.open(os.devnull, os.O_WRONLY)
>         ui = self.ui
> -        for (ch, fp, fd), (cn, fn, _mode) in zip(self._oldios, _iochannels):
> +        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
> @@ -443,6 +446,12 @@ class chgcmdserver(commandserver.server)
>                 newfp.close()
>             # restore original fd: fp is open again
>             try:
> +                if 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.
> +                    os.dup2(nullfd, fp.fileno())
> +                    fp.flush()
>                 os.dup2(fd, fp.fileno())
>             except OSError as err:
>                 # According to issue6330, running chg on heavy loaded systems
> @@ -459,6 +468,7 @@ class chgcmdserver(commandserver.server)
>             os.close(fd)
>             setattr(self, cn, ch)
>             setattr(ui, fn, fp)
> +        os.close(nullfd)
>         del self._oldios[:]
> 
>     def validate(self):
> diff --git a/tests/test-chg.t b/tests/test-chg.t
> --- a/tests/test-chg.t
> +++ b/tests/test-chg.t
> @@ -152,6 +152,49 @@ chg waits for pager if runcommand raises
>   crash-pager: going to crash
>   [255]
> 
> +no stdout data should be printed after pager quits, and the buffered data
> +should never persist (issue6207)
> +
> +"killed!" may be printed if terminated by SIGPIPE, which isn't important
> +in this test.
> +
> +  $ cat > $TESTTMP/bulkwrite.py <<'EOF'
> +  > import time
> +  > from mercurial import error, registrar
> +  > cmdtable = {}
> +  > command = registrar.command(cmdtable)
> +  > @command(b'bulkwrite')
> +  > def bulkwrite(ui, repo, *pats, **opts):
> +  >     ui.write(b'going to write massive data\n')
> +  >     ui.flush()
> +  >     t = time.time()
> +  >     while time.time() - t < 2:
> +  >         ui.write(b'x' * 1023 + b'\n')  # will be interrupted by SIGPIPE
> +  >     raise error.Abort(b"write() doesn't block")
> +  > EOF
> +
> +  $ cat > $TESTTMP/fakepager.py <<'EOF'
> +  > import sys
> +  > import time
> +  > sys.stdout.write('paged! %r\n' % sys.stdin.readline())
> +  > time.sleep(1)  # new data will be written
> +  > EOF
> +
> +  $ cat >> .hg/hgrc <<EOF
> +  > [extensions]
> +  > bulkwrite = $TESTTMP/bulkwrite.py
> +  > EOF
> +
> +  $ chg bulkwrite --pager=on --color no --config ui.formatted=True
> +  paged! 'going to write massive data\n'
> +  killed! (?)
> +  [255]
> +
> +  $ chg bulkwrite --pager=on --color no --config ui.formatted=True
> +  paged! 'going to write massive data\n'
> +  killed! (?)
> +  [255]
> +
>   $ cd ..
> 
> server lifecycle
> _______________________________________________
> 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
@@ -434,8 +434,11 @@  class chgcmdserver(commandserver.server)
             self._oldios.append((ch, fp, fd))
 
     def _restoreio(self):
+        if not self._oldios:
+            return
+        nullfd = os.open(os.devnull, os.O_WRONLY)
         ui = self.ui
-        for (ch, fp, fd), (cn, fn, _mode) in zip(self._oldios, _iochannels):
+        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
@@ -443,6 +446,12 @@  class chgcmdserver(commandserver.server)
                 newfp.close()
             # restore original fd: fp is open again
             try:
+                if 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.
+                    os.dup2(nullfd, fp.fileno())
+                    fp.flush()
                 os.dup2(fd, fp.fileno())
             except OSError as err:
                 # According to issue6330, running chg on heavy loaded systems
@@ -459,6 +468,7 @@  class chgcmdserver(commandserver.server)
             os.close(fd)
             setattr(self, cn, ch)
             setattr(ui, fn, fp)
+        os.close(nullfd)
         del self._oldios[:]
 
     def validate(self):
diff --git a/tests/test-chg.t b/tests/test-chg.t
--- a/tests/test-chg.t
+++ b/tests/test-chg.t
@@ -152,6 +152,49 @@  chg waits for pager if runcommand raises
   crash-pager: going to crash
   [255]
 
+no stdout data should be printed after pager quits, and the buffered data
+should never persist (issue6207)
+
+"killed!" may be printed if terminated by SIGPIPE, which isn't important
+in this test.
+
+  $ cat > $TESTTMP/bulkwrite.py <<'EOF'
+  > import time
+  > from mercurial import error, registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command(b'bulkwrite')
+  > def bulkwrite(ui, repo, *pats, **opts):
+  >     ui.write(b'going to write massive data\n')
+  >     ui.flush()
+  >     t = time.time()
+  >     while time.time() - t < 2:
+  >         ui.write(b'x' * 1023 + b'\n')  # will be interrupted by SIGPIPE
+  >     raise error.Abort(b"write() doesn't block")
+  > EOF
+
+  $ cat > $TESTTMP/fakepager.py <<'EOF'
+  > import sys
+  > import time
+  > sys.stdout.write('paged! %r\n' % sys.stdin.readline())
+  > time.sleep(1)  # new data will be written
+  > EOF
+
+  $ cat >> .hg/hgrc <<EOF
+  > [extensions]
+  > bulkwrite = $TESTTMP/bulkwrite.py
+  > EOF
+
+  $ chg bulkwrite --pager=on --color no --config ui.formatted=True
+  paged! 'going to write massive data\n'
+  killed! (?)
+  [255]
+
+  $ chg bulkwrite --pager=on --color no --config ui.formatted=True
+  paged! 'going to write massive data\n'
+  killed! (?)
+  [255]
+
   $ cd ..
 
 server lifecycle