This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1428

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

Authored by lothiraldan on Nov 15 2017, 10:44 AM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Nov 15 2017, 10:44 AM
yuja requested changes to this revision.Nov 16 2017, 8:34 AM
yuja added a subscriber: yuja.
yuja added inline comments.
hgext/logtoprocess.py
99

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().

This revision now requires changes to proceed.Nov 16 2017, 8:34 AM
lothiraldan added inline comments.Nov 17 2017, 3:03 AM
hgext/logtoprocess.py
99

Thx for the catch, I will make the change.

lothiraldan updated this revision to Diff 3589.Nov 17 2017, 3:25 AM
lothiraldan marked 2 inline comments as done.Nov 17 2017, 3:25 AM
lothiraldan updated this revision to Diff 3917.Nov 28 2017, 3:36 PM
lothiraldan updated this revision to Diff 4469.Dec 15 2017, 5:11 AM

Revision Contents
Changeset List

PathPackages
Mhgext/logtoprocess.py (13 lines)
Mtests/test-logtoprocess.t (2 lines)

Diff 4469

hgext/logtoprocess.py

return return
# subprocess.Popen() forks again, all we need to add is # subprocess.Popen() forks again, all we need to add is
# flag the new process as a new session. # flag the new process as a new session.
if sys.version_info < (3, 2): if sys.version_info < (3, 2):
newsession = {'preexec_fn': os.setsid} newsession = {'preexec_fn': os.setsid}
else: else:
newsession = {'start_new_session': True} newsession = {'start_new_session': True}
try: try:
# connect stdin to devnull to make sure the subprocess can't # connect std* to devnull to make sure the subprocess can't
# muck up that stream for mercurial. # 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
nullrfd = open(os.devnull, 'r')
nullwfd = open(os.devnull, 'w')
subprocess.Popen( subprocess.Popen(
script, shell=True, stdin=open(os.devnull, 'r'), env=env, script, shell=True, stdin=nullrfd,
close_fds=True, **newsession) stdout=nullwfd, stderr=nullwfd,
env=env, close_fds=True, **newsession)
yujaUnsubmitted
Done

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().

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

Thx for the catch, I will make the change.

lothiraldan: Thx for the catch, I will make the change.
finally: finally:
# mission accomplished, this child needs to exit and not # mission accomplished, this child needs to exit and not
# continue the hg process here. # continue the hg process here.
os._exit(0) os._exit(0)
class logtoprocessui(ui.__class__): class logtoprocessui(ui.__class__):
def log(self, event, *msg, **opts): def log(self, event, *msg, **opts):
"""Map log events to external commands """Map log events to external commands

tests/test-logtoprocess.t

> [logtoprocess] > [logtoprocess]
> commandfinish=$TESTTMP/wait-output.sh > commandfinish=$TESTTMP/wait-output.sh
> EOF > EOF
$ hg version -q --pager=always $ hg version -q --pager=always
Mercurial Distributed SCM (version *) (glob) Mercurial Distributed SCM (version *) (glob)
$ touch $TESTTMP/wait-for-touched $ touch $TESTTMP/wait-for-touched
$ sleep 0.2 $ sleep 0.2
$ test -f $TESTTMP/touched && echo "SUCCESS Pager is waiting on ltp" || echo "FAIL Pager is waiting on ltp" $ 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