Page MenuHomePhabricator

procutil: assign stdio objects if they are None
Needs ReviewPublic

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


Group Reviewers

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


Diff Detail

rHG Mercurial
No Linters Available
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

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/ ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r' encoding='UTF-8'>
  sys.stdin = io.TextIOWrapper(

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 and, including reassigning sys.std* in 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')
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 patches sys.std* directly