Patchwork [1,of,5] pager: stdout is line buffered by default

login
register
mail settings
Submitter Simon Farnsworth
Date Jan. 19, 2017, 7:02 p.m.
Message ID <76123ae2e0ccaa58db3d.1484852527@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18264/
State Deferred
Headers show

Comments

Simon Farnsworth - Jan. 19, 2017, 7:02 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1484835774 28800
#      Thu Jan 19 06:22:54 2017 -0800
# Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16
# Parent  036c37bd3ec189480647ff568cee9e0b43a5bc81
pager: stdout is line buffered by default

pager only starts when ui.formatted() is true. In normal operation,
when ui.formatted() is true, stdout is line buffered anyway.

The code here doesn't actually do what the commit message in changeset
62c5e937 claims it will do; sys.stdout is not yet pager.stdin when we
reopen, so we affect the original sys.stdout's buffering mode. It's the dup2
that puts pager.stdin's fd into sys.stdout's fd, and that doesn't affect
buffering.
via Mercurial-devel - Jan. 19, 2017, 7:16 p.m.
On Thu, Jan 19, 2017 at 11:02 AM, Simon Farnsworth <simonfar@fb.com> wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1484835774 28800
> #      Thu Jan 19 06:22:54 2017 -0800
> # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16
> # Parent  036c37bd3ec189480647ff568cee9e0b43a5bc81
> pager: stdout is line buffered by default

We're in a code freeze and accept patches for the stable branch only,
so please resend this after the freeze is over (~ Feb 3).
Yuya Nishihara - Jan. 20, 2017, 12:56 p.m.
On Thu, 19 Jan 2017 11:02:07 -0800, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1484835774 28800
> #      Thu Jan 19 06:22:54 2017 -0800
> # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16
> # Parent  036c37bd3ec189480647ff568cee9e0b43a5bc81
> pager: stdout is line buffered by default
> 
> pager only starts when ui.formatted() is true. In normal operation,
> when ui.formatted() is true, stdout is line buffered anyway.

Unfortunately that isn't always true. The buffering mode of FILE object is
decided on the first fwrite() (or perhaps fflush()) as far as on glibc. We
generally attach the pager fd before writing anything, which makes the stdout
fully-buffered.

> The code here doesn't actually do what the commit message in changeset
> 62c5e937 claims it will do; sys.stdout is not yet pager.stdin when we
> reopen, so we affect the original sys.stdout's buffering mode. It's the dup2
> that puts pager.stdin's fd into sys.stdout's fd, and that doesn't affect
> buffering.

No. AFAIK, the buffering mode is defined per FILE object, not per file
descriptor.
Simon Farnsworth - Jan. 20, 2017, 3:23 p.m.
On 20/01/2017 12:56, Yuya Nishihara wrote:
> On Thu, 19 Jan 2017 11:02:07 -0800, Simon Farnsworth wrote:
>> # HG changeset patch
>> # User Simon Farnsworth <simonfar@fb.com>
>> # Date 1484835774 28800
>> #      Thu Jan 19 06:22:54 2017 -0800
>> # Node ID 76123ae2e0ccaa58db3d4fc26b75b7251e13ad16
>> # Parent  036c37bd3ec189480647ff568cee9e0b43a5bc81
>> pager: stdout is line buffered by default
>>
>> pager only starts when ui.formatted() is true. In normal operation,
>> when ui.formatted() is true, stdout is line buffered anyway.
>
> Unfortunately that isn't always true. The buffering mode of FILE object is
> decided on the first fwrite() (or perhaps fflush()) as far as on glibc. We
> generally attach the pager fd before writing anything, which makes the stdout
> fully-buffered.
>
>> The code here doesn't actually do what the commit message in changeset
>> 62c5e937 claims it will do; sys.stdout is not yet pager.stdin when we
>> reopen, so we affect the original sys.stdout's buffering mode. It's the dup2
>> that puts pager.stdin's fd into sys.stdout's fd, and that doesn't affect
>> buffering.
>
> No. AFAIK, the buffering mode is defined per FILE object, not per file
> descriptor.
>

Argh. I've written a test program, and this is true of glibc (thanks for 
the unclear documentation, glibc guys) - I'll improve the comment when I 
resubmit after the freeze.

Patch

diff --git a/hgext/pager.py b/hgext/pager.py
--- a/hgext/pager.py
+++ b/hgext/pager.py
@@ -87,14 +87,10 @@ 
                              close_fds=util.closefds, stdin=subprocess.PIPE,
                              stdout=util.stdout, stderr=util.stderr)
 
-    # back up original file objects and descriptors
-    olduifout = ui.fout
-    oldstdout = util.stdout
+    # back up original file descriptors
     stdoutfd = os.dup(util.stdout.fileno())
     stderrfd = os.dup(util.stderr.fileno())
 
-    # create new line-buffered stdout so that output can show up immediately
-    ui.fout = util.stdout = newstdout = os.fdopen(util.stdout.fileno(), 'wb', 1)
     os.dup2(pager.stdin.fileno(), util.stdout.fileno())
     if ui._isatty(util.stderr):
         os.dup2(pager.stdin.fileno(), util.stderr.fileno())
@@ -103,15 +99,12 @@ 
     def killpager():
         if util.safehasattr(signal, "SIGINT"):
             signal.signal(signal.SIGINT, signal.SIG_IGN)
-        pager.stdin.close()
-        ui.fout = olduifout
-        util.stdout = oldstdout
-        # close new stdout while it's associated with pager; otherwise stdout
-        # fd would be closed when newstdout is deleted
-        newstdout.close()
-        # restore original fds: stdout is open again
+        # restore original fds
         os.dup2(stdoutfd, util.stdout.fileno())
         os.dup2(stderrfd, util.stderr.fileno())
+        # close pager's stdin so that it knows we're not going to send any more
+        # data
+        pager.stdin.close()
         pager.wait()
 
 def uisetup(ui):