This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4816

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

Authored by lothiraldan on Oct 1 2018, 6:18 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGc4a3d3c67c4f: logtoprocess: connect all fds to /dev/null to avoid bad interaction with pager
rHG030f40677db6: logtoprocess: connect all fds to /dev/null to avoid bad interaction with pager
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lothiraldan created this revision.Oct 1 2018, 6:18 AM
Closed by commit rHG030f40677db6: logtoprocess: connect all fds to /dev/null to avoid bad interaction with pager (authored by lothiraldan). · Explain WhyOct 1 2018, 7:42 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

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

Diff 11511

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(
procutil.tonativestr(script), procutil.tonativestr(script),
shell=True, stdin=open(os.devnull, 'r'), shell=True, stdin=nullrfd,
stdout=nullwfd, stderr=nullwfd,
env=procutil.tonativeenv(env), env=procutil.tonativeenv(env),
close_fds=True, **newsession) close_fds=True, **newsession)
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__):

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 not waiting on ltp" || echo "FAIL Pager is waiting on ltp" $ test -f $TESTTMP/touched && echo "SUCCESS Pager is not waiting on ltp" || echo "FAIL Pager is waiting on ltp"
FAIL Pager is waiting on ltp SUCCESS Pager is not waiting on ltp