Patchwork D1428: logtoprocess: connect all fds to /dev/null to avoid bad interaction with pager

login
register
mail settings
Submitter phabricator
Date Nov. 15, 2017, 3:44 p.m.
Message ID <differential-rev-PHID-DREV-ug4bvz7hvrkxig2vc2e4-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25587/
State New
Headers show

Comments

phabricator - Nov. 15, 2017, 3:44 p.m.
lothiraldan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We detected that pager is waiting for log-to-process script to finish, which
  is annoying when adding a script on commandfinish that does an HTTP push.
  
  There seems to be no workaround on the script side and it will make the
  behavior on Linux/MacOS closer to the Windows behavior.
  
  The drawback is that it makes the related tests more flaky as log-to-process
  outputs are now really asynchronous.
  
  If it's considered a BC change, another option would be to add a config option
  for this new behavior. I personally think that the different behavior between
  Windows and Linux is confusing and that it's a bug I would be fine with a new
  config option.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1428

AFFECTED FILES
  hgext/logtoprocess.py
  tests/test-logtoprocess.t

CHANGE DETAILS




To: lothiraldan, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 16, 2017, 1:34 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> logtoprocess.py:89
> +                    stdout=open(os.devnull, 'r'),
> +                    stderr=open(os.devnull, 'r'),
> +                    env=env, close_fds=True, **newsession)

stdout and stderr should be writable. We can simply pass a file
descriptor open with O_RDWR.

  nullfd = os.open(os.devnull, os.O_RDWR)
  Popen(stdin=nullfd, stdout=nullfd, stderr=nullfd)

and `os.close(nullfd)` though it doesn't mater since the process
is terminated with `_exit()`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1428

To: lothiraldan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Nov. 17, 2017, 8:03 a.m.
lothiraldan added inline comments.

INLINE COMMENTS

> yuja wrote in logtoprocess.py:89
> stdout and stderr should be writable. We can simply pass a file
> descriptor open with O_RDWR.
> 
>   nullfd = os.open(os.devnull, os.O_RDWR)
>   Popen(stdin=nullfd, stdout=nullfd, stderr=nullfd)
> 
> and `os.close(nullfd)` though it doesn't mater since the process
> is terminated with `_exit()`.

Thx for the catch, I will make the change.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1428

To: lothiraldan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-logtoprocess.t b/tests/test-logtoprocess.t
--- a/tests/test-logtoprocess.t
+++ b/tests/test-logtoprocess.t
@@ -122,4 +122,4 @@ 
   $ touch $TESTTMP/wait-for-touched
   $ sleep 0.2
   $ test -f $TESTTMP/touched && echo "SUCCESS Pager is waiting on ltp" || echo "FAIL Pager is waiting on ltp"
-  FAIL Pager is waiting on ltp
+  SUCCESS Pager is waiting on ltp
diff --git a/hgext/logtoprocess.py b/hgext/logtoprocess.py
--- a/hgext/logtoprocess.py
+++ b/hgext/logtoprocess.py
@@ -79,11 +79,15 @@ 
             else:
                 newsession = {'start_new_session': True}
             try:
-                # connect stdin to devnull to make sure the subprocess can't
-                # muck up that stream for mercurial.
+                # connect std* to devnull to make sure the subprocess can't
+                # muck up these stream for mercurial.
+                # Connect all the streams to be more close to Windows behavior
+                # and pager will wait for scripts to end if we don't do that
                 subprocess.Popen(
-                    script, shell=shell, stdin=open(os.devnull, 'r'), env=env,
-                    close_fds=True, **newsession)
+                    script, shell=True, stdin=open(os.devnull, 'r'),
+                    stdout=open(os.devnull, 'r'),
+                    stderr=open(os.devnull, 'r'),
+                    env=env, close_fds=True, **newsession)
             finally:
                 # mission accomplished, this child needs to exit and not
                 # continue the hg process here.