Patchwork [1,of,5,chg-tests-fix,V2] procutils: don't try to get `.buffer` if sys.stdin is None

login
register
mail settings
Submitter Pulkit Goyal
Date Dec. 8, 2020, 1:42 p.m.
Message ID <13496bf926221a034c89.1607434940@DESKTOP-STQPTJK>
Download mbox | patch
Permalink /patch/47835/
State New
Headers show

Comments

Pulkit Goyal - Dec. 8, 2020, 1:42 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1606897517 -19800
#      Wed Dec 02 13:55:17 2020 +0530
# Node ID 13496bf926221a034c890135954fdc72c895f6d7
# Parent  8b20d469a0c819c2ade8635c25e9fcf0af553796
# EXP-Topic chg-test
procutils: don't try to get `.buffer` if sys.stdin is None

While hunting down following test failure of test-chg.t on Python 3, I stumbled
the case when `.buffer` is not available as sys.stdin is None.

	--- /home/pulkit/repo/hg-committed/tests/test-chg.t
	+++ /home/pulkit/repo/hg-committed/tests/test-chg.t.err
	@@ -203,7 +203,31 @@
	   $ CHGDEBUG=1 chg version -q 0<&-
		 chg: debug: * stdio fds are missing (glob)
	     chg: debug: * execute original hg (glob)
		 -  Mercurial Distributed SCM * (glob)
		 +  Traceback (most recent call last):
		 +    File "/tmp/hgtests.avspvsq4/install/bin/hg", line 43, in <module>
		 +      dispatch.run()
		 +    File "/usr/lib/python3.6/importlib/util.py", line 233, in
		 __getattribute__
		 +      self.__spec__.loader.exec_module(self)
		 +    File "<frozen importlib._bootstrap_external>", line 678, in
		 exec_module
		 +    File "<frozen importlib._bootstrap>", line 219, in
		 _call_with_frames_removed
		 +    File
		 "/tmp/hgtests.avspvsq4/install/lib/python/mercurial/dispatch.py", line
		 726, in <module>
		 +      class lazyaliasentry(object):
		 +    File
		 "/tmp/hgtests.avspvsq4/install/lib/python/mercurial/dispatch.py", line
		 737, in lazyaliasentry
		 +      @util.propertycache
		 +    File "/usr/lib/python3.6/importlib/util.py", line 233, in
		 __getattribute__
		 +      self.__spec__.loader.exec_module(self)
		 +    File "<frozen importlib._bootstrap_external>", line 678, in
		 exec_module
		 +    File "<frozen importlib._bootstrap>", line 219, in
		 _call_with_frames_removed
		 +    File "/tmp/hgtests.avspvsq4/install/lib/python/mercurial/util.py",
		 line 3473, in <module>
		 +      f=procutil.stderr,
		 +    File "/usr/lib/python3.6/importlib/util.py", line 233, in
		 __getattribute__
		 +      self.__spec__.loader.exec_module(self)
		 +    File "<frozen importlib._bootstrap_external>", line 678, in
		 exec_module
		 +    File "<frozen importlib._bootstrap>", line 219, in
		 _call_with_frames_removed
		 +    File
		 "/tmp/hgtests.avspvsq4/install/lib/python/mercurial/utils/procutil.py",
		 line 127, in <module>
		 +      stdin = sys.stdin.buffer
		 +  AttributeError: 'NoneType' object has no attribute 'buffer'
		 +  [1]

		  server lifecycle
		   ----------------

Differential Revision: https://phab.mercurial-scm.org/D9500
Yuya Nishihara - Dec. 9, 2020, 10:46 a.m.
On Tue, 08 Dec 2020 19:12:20 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1606897517 -19800
> #      Wed Dec 02 13:55:17 2020 +0530
> # Node ID 13496bf926221a034c890135954fdc72c895f6d7
> # Parent  8b20d469a0c819c2ade8635c25e9fcf0af553796
> # EXP-Topic chg-test
> procutils: don't try to get `.buffer` if sys.stdin is None

> diff -r 8b20d469a0c8 -r 13496bf92622 mercurial/utils/procutil.py
> --- a/mercurial/utils/procutil.py	Fri Apr 03 20:30:36 2020 +0530
> +++ b/mercurial/utils/procutil.py	Wed Dec 02 13:55:17 2020 +0530
> @@ -124,7 +124,9 @@
>      # Python 3 implements its own I/O streams.
>      # TODO: .buffer might not exist if std streams were replaced; we'll need
>      # a silly wrapper to make a bytes stream backed by a unicode one.
> -    stdin = sys.stdin.buffer
> +
> +    # sys.stdin can be None
> +    stdin = sys.stdin.buffer if sys.stdin else os.fdopen(0, 'rb')

Oops, os.fdopen() fails if fd=0 isn't available, so we'll need other
workaround. Sorry.

  % python3 -c 'import os; os.fdopen(0, "rb")' 0<&-
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
    File "/usr/lib/python3.9/os.py", line 1023, in fdopen
      return io.open(fd, *args, **kwargs)
  OSError: [Errno 9] Bad file descriptor

We can simulate Py2 behavior by opening /dev/null and closing the underlying
fd, but that might be a bit hacky.

  f = open(os.devnull, 'rb')
  os.close(f.fileno())

Any thoughts?

For stdout/stderr, we do need something. Adding null check to procutil
and dispatch isn't enough.

  % python3 hg version  1<&-
  ...
  Traceback (most recent call last):
    File "mercurial/dispatch.py", line 434, in _runcatchfunc
      return _dispatch(req)
    File "mercurial/dispatch.py", line 1039, in _dispatch
      extensions.loadall(lui)
    File "mercurial/extensions.py", line 310, in loadall
      ui.warn(
    File "mercurial/ui.py", line 1766, in warn
      self._writemsg(self._fmsgerr, type=b'warning', *msg, **opts)
    File "mercurial/ui.py", line 1241, in _writemsg
      _writemsgwith(self._write, dest, *args, **opts)
    File "mercurial/ui.py", line 2381, in _writemsgwith
      write(dest, *args, **opts)
    File "mercurial/ui.py", line 1184, in _write
      self._writenobuf(dest, *args, **opts)
    File "mercurial/ui.py", line 1196, in _writenobuf
      self._fout.flush()
  AttributeError: 'NoneType' object has no attribute 'flush'

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "mercurial/dispatch.py", line 453, in _callcatch
      return scmutil.callcatch(ui, func)
    File "mercurial/scmutil.py", line 155, in callcatch
      return func()
    File "mercurial/dispatch.py", line 436, in _runcatchfunc
      ui.flush()
    File "mercurial/ui.py", line 1253, in flush
      self._fout.flush()
  AttributeError: 'NoneType' object has no attribute 'flush'

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "mercurial/dispatch.py", line 268, in dispatch
      ret = _runcatch(req) or 0
    File "mercurial/dispatch.py", line 444, in _runcatch
      return _callcatch(ui, _runcatchfunc)
    File "mercurial/dispatch.py", line 494, in _callcatch
      if not handlecommandexception(ui):
    File "mercurial/dispatch.py", line 1346, in handlecommandexception
      ui.warn(warning)
    File "mercurial/ui.py", line 1766, in warn
      self._writemsg(self._fmsgerr, type=b'warning', *msg, **opts)
    File "mercurial/ui.py", line 1241, in _writemsg
      _writemsgwith(self._write, dest, *args, **opts)
    File "mercurial/ui.py", line 2381, in _writemsgwith
      write(dest, *args, **opts)
    File "mercurial/ui.py", line 1184, in _write
      self._writenobuf(dest, *args, **opts)
    File "mercurial/ui.py", line 1196, in _writenobuf
      self._fout.flush()
  AttributeError: 'NoneType' object has no attribute 'flush'

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "hg", line 59, in <module>
      dispatch.run()
    File "mercurial/dispatch.py", line 115, in run
      status = dispatch(req)
    File "mercurial/dispatch.py", line 291, in dispatch
      req.ui.flush()  # record blocked times
    File "mercurial/ui.py", line 1253, in flush
      self._fout.flush()
  AttributeError: 'NoneType' object has no attribute 'flush'
Pulkit Goyal - Dec. 10, 2020, 8:07 a.m.
On Wed, Dec 9, 2020 at 4:17 PM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Tue, 08 Dec 2020 19:12:20 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1606897517 -19800
> > #      Wed Dec 02 13:55:17 2020 +0530
> > # Node ID 13496bf926221a034c890135954fdc72c895f6d7
> > # Parent  8b20d469a0c819c2ade8635c25e9fcf0af553796
> > # EXP-Topic chg-test
> > procutils: don't try to get `.buffer` if sys.stdin is None
>
> > diff -r 8b20d469a0c8 -r 13496bf92622 mercurial/utils/procutil.py
> > --- a/mercurial/utils/procutil.py     Fri Apr 03 20:30:36 2020 +0530
> > +++ b/mercurial/utils/procutil.py     Wed Dec 02 13:55:17 2020 +0530
> > @@ -124,7 +124,9 @@
> >      # Python 3 implements its own I/O streams.
> >      # TODO: .buffer might not exist if std streams were replaced; we'll need
> >      # a silly wrapper to make a bytes stream backed by a unicode one.
> > -    stdin = sys.stdin.buffer
> > +
> > +    # sys.stdin can be None
> > +    stdin = sys.stdin.buffer if sys.stdin else os.fdopen(0, 'rb')
>
> Oops, os.fdopen() fails if fd=0 isn't available, so we'll need other
> workaround. Sorry.
>
>   % python3 -c 'import os; os.fdopen(0, "rb")' 0<&-
>   Traceback (most recent call last):
>     File "<string>", line 1, in <module>
>     File "/usr/lib/python3.9/os.py", line 1023, in fdopen
>       return io.open(fd, *args, **kwargs)
>   OSError: [Errno 9] Bad file descriptor
>
> We can simulate Py2 behavior by opening /dev/null and closing the underlying
> fd, but that might be a bit hacky.
>
>   f = open(os.devnull, 'rb')
>   os.close(f.fileno())
>
> Any thoughts?
>
> For stdout/stderr, we do need something. Adding null check to procutil
> and dispatch isn't enough.

Seems like we can introduce a wrapper around sys.stdin/out/err. I
guess procutil.stdin/out/err is that wrapper only.
I am not sure what's correct because my understanding in this area is limited.

(My V1 patches got pushed, sorry about that. Actually I first sent
them on phabricator and realized later that I should have a look from
you. I forgot to update phabricator about conversation here and they
are pushed. I will update this series to be based on those patches.)
>
>   % python3 hg version  1<&-
>   ...
>   Traceback (most recent call last):
>     File "mercurial/dispatch.py", line 434, in _runcatchfunc
>       return _dispatch(req)
>     File "mercurial/dispatch.py", line 1039, in _dispatch
>       extensions.loadall(lui)
>     File "mercurial/extensions.py", line 310, in loadall
>       ui.warn(
>     File "mercurial/ui.py", line 1766, in warn
>       self._writemsg(self._fmsgerr, type=b'warning', *msg, **opts)
>     File "mercurial/ui.py", line 1241, in _writemsg
>       _writemsgwith(self._write, dest, *args, **opts)
>     File "mercurial/ui.py", line 2381, in _writemsgwith
>       write(dest, *args, **opts)
>     File "mercurial/ui.py", line 1184, in _write
>       self._writenobuf(dest, *args, **opts)
>     File "mercurial/ui.py", line 1196, in _writenobuf
>       self._fout.flush()
>   AttributeError: 'NoneType' object has no attribute 'flush'
>
>   During handling of the above exception, another exception occurred:
>
>   Traceback (most recent call last):
>     File "mercurial/dispatch.py", line 453, in _callcatch
>       return scmutil.callcatch(ui, func)
>     File "mercurial/scmutil.py", line 155, in callcatch
>       return func()
>     File "mercurial/dispatch.py", line 436, in _runcatchfunc
>       ui.flush()
>     File "mercurial/ui.py", line 1253, in flush
>       self._fout.flush()
>   AttributeError: 'NoneType' object has no attribute 'flush'
>
>   During handling of the above exception, another exception occurred:
>
>   Traceback (most recent call last):
>     File "mercurial/dispatch.py", line 268, in dispatch
>       ret = _runcatch(req) or 0
>     File "mercurial/dispatch.py", line 444, in _runcatch
>       return _callcatch(ui, _runcatchfunc)
>     File "mercurial/dispatch.py", line 494, in _callcatch
>       if not handlecommandexception(ui):
>     File "mercurial/dispatch.py", line 1346, in handlecommandexception
>       ui.warn(warning)
>     File "mercurial/ui.py", line 1766, in warn
>       self._writemsg(self._fmsgerr, type=b'warning', *msg, **opts)
>     File "mercurial/ui.py", line 1241, in _writemsg
>       _writemsgwith(self._write, dest, *args, **opts)
>     File "mercurial/ui.py", line 2381, in _writemsgwith
>       write(dest, *args, **opts)
>     File "mercurial/ui.py", line 1184, in _write
>       self._writenobuf(dest, *args, **opts)
>     File "mercurial/ui.py", line 1196, in _writenobuf
>       self._fout.flush()
>   AttributeError: 'NoneType' object has no attribute 'flush'
>
>   During handling of the above exception, another exception occurred:
>
>   Traceback (most recent call last):
>     File "hg", line 59, in <module>
>       dispatch.run()
>     File "mercurial/dispatch.py", line 115, in run
>       status = dispatch(req)
>     File "mercurial/dispatch.py", line 291, in dispatch
>       req.ui.flush()  # record blocked times
>     File "mercurial/ui.py", line 1253, in flush
>       self._fout.flush()
>   AttributeError: 'NoneType' object has no attribute 'flush'

Patch

diff -r 8b20d469a0c8 -r 13496bf92622 mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py	Fri Apr 03 20:30:36 2020 +0530
+++ b/mercurial/utils/procutil.py	Wed Dec 02 13:55:17 2020 +0530
@@ -124,7 +124,9 @@ 
     # Python 3 implements its own I/O streams.
     # TODO: .buffer might not exist if std streams were replaced; we'll need
     # a silly wrapper to make a bytes stream backed by a unicode one.
-    stdin = sys.stdin.buffer
+
+    # sys.stdin can be None
+    stdin = sys.stdin.buffer if sys.stdin else os.fdopen(0, 'rb')
     stdout = _make_write_all(sys.stdout.buffer)
     stderr = _make_write_all(sys.stderr.buffer)
     if pycompat.iswindows: