This is an archive of the discontinued Mercurial Phabricator instance.

procutil: assign stdio objects if they are None
AbandonedPublic

Authored by quark on Sep 18 2020, 8:07 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

On Python 3 stdio objects can be None. That causes crashes. Fix it by opening
devnull automatically.

stdin can be None by using 0<&- in bash, or spawning processes less
carefully, for example, watchman used to cause such None stdin [1] (note:
None is only observable on Python 3).

[1]: https://github.com/facebook/watchman/commit/d241978aaa6b6d7c5b7260bc9e6d699d3a1cea53

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

quark created this revision.Sep 18 2020, 8:07 PM
mjacob added a subscriber: mjacob.Sep 19 2020, 12:41 AM

I think it’s reasonable that Mercurial expects that the stdio streams are initialized properly. Is there a use case for running Mercurial with FDs 0, 1 or 2 set to non-standard streams?

mjacob added inline comments.Sep 19 2020, 1:12 AM
mercurial/utils/procutil.py
119–121

The created streams are replaced later by initstdio(), causing them to get garbage-collected without getting closed explicitly.

If you set environment variable PYTHONWARNINGS=always, you’ll get something like

/home/manu/vcs/hg/mercurial/dispatch.py:189: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r' encoding='UTF-8'>
  sys.stdin = io.TextIOWrapper(
121

Is there a reason for not opening a dedicated stream for stderr?

This patch breaks my brain because our mucking about with sys.std* is wonky. We are doing really hacky things in both procutil.py and dispatch.py, including reassigning sys.std* in dispatch.py before this patch. I'd strongly prefer to have that logic consolidated.

As for the Python warning, that's super annoying. But I'm not sure we can do anything about that since we don't have an obvious place to insert an __exit__ handler. This is untested, but the best I can come up with is:

if not sys.stdin:
    sys.stdin = open(os.devnull, 'r')
    atexit.register(sys.stdin.close)
quark updated this revision to Diff 23030.Oct 2 2020, 10:45 PM
quark added a comment.Oct 2 2020, 10:55 PM

Updated to remove the warning and open a separate stderr.

quark updated this revision to Diff 23031.Oct 2 2020, 11:17 PM

Updated so only dispatch.py patches sys.std* directly

yuja added a subscriber: yuja.Dec 18 2020, 9:55 PM

As Pulkit pointed out, I and Pulkit made another version in parallel.

https://patchwork.mercurial-scm.org/patch/47937/
https://patchwork.mercurial-scm.org/patch/47938/
https://patchwork.mercurial-scm.org/patch/47939/

sys.std* are left unmodified. procutil.std* are fixed up to raise EBADF.

marmoute requested changes to this revision.Jan 8 2021, 11:07 AM
marmoute added a subscriber: marmoute.

It looks like the alternative got in.

This revision now requires changes to proceed.Jan 8 2021, 11:07 AM
quark abandoned this revision.Jan 16 2021, 11:39 PM