Patchwork [4,of,5] py3: stop subscripting socket.error

login
register
mail settings
Submitter Matt Harbison
Date Dec. 10, 2018, 4:39 a.m.
Message ID <op.ztshv7he9lwrgf@envy>
Download mbox | patch
Permalink /patch/37047/
State New
Headers show

Comments

Matt Harbison - Dec. 10, 2018, 4:39 a.m.
On Sun, 09 Dec 2018 22:44:36 -0500, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1544402454 18000
> #      Sun Dec 09 19:40:54 2018 -0500
> # Node ID e5b7d60068537baa1ffeeca4e1a81f7498d0d48e
> # Parent  8f35bbc97ab5ec0ab5072900b9a70cea8fe720d0
> py3: stop subscripting socket.error
>
> In 3.3 and later, this is now an alias for OSError.  I hacked up the  
> server code
> enough that I was able to trigger the exception handler in server.py from
> test-http-bundle1.t.

The details here are that I noticed a lot of `hg serve` instances failing  
to spawn children on Windows.  So I added this:

      # Since Python 3 converts argv to wchar_t type by Py_DecodeLocale()  
on Unix,
      # we can use os.fsencode() to get back bytes argv.

Swallowing the first error in windows.py "fixed" over 40 tests, so what's  
the right way to do this?
Yuya Nishihara - Dec. 10, 2018, 12:12 p.m.
On Sun, 09 Dec 2018 23:39:33 -0500, Matt Harbison wrote:
> I ended up with similar stdio errors trying to track down the bad http  
> status failures:
> 
> Traceback (most recent call last):
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1033, in _writenobuf
>      dest.flush()
> OSError: [WinError 1] Incorrect function
> 
> During handling of the above exception, another exception occurred:
> 
> Traceback (most recent call last):
>    File "c:\Users\Matt\hg\mercurial\hgweb\server.py", line 192, in do_hgweb
>      for chunk in self.server.application(env, self._start_response):
>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 310, in  
> run_wsgi
>      for r in self._runwsgi(req, res, repo):
>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 430, in  
> _runwsgi
>      return getattr(webcommands, cmd)(rctx)
>    File "c:\Users\Matt\hg\mercurial\hgweb\webcommands.py", line 861, in  
> comparison
>      context = parsecontext(web.config('web', 'comparisoncontext', '5'))
>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 117, in config
>      untrusted=untrusted)
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 517, in config
>      untrusted=untrusted)
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 536, in _config
>      self.develwarn(msg, 2, 'warn-config-unknown')
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1790, in develwarn
>      % (msg, fname, lineno, fmsg))
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 996, in write_err
>      self._write(self._ferr, *args, **opts)
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1006, in _write
>      self._writenobuf(dest, *args, **opts)
>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1039, in _writenobuf
>      raise error.StdioError(err)
> mercurial.error.StdioError: [Errno 22] Incorrect function
> 
> I see that there's a TODO for pycompat.{stdin,stdout,stderr} to use a  
> silly wrapper instead of .buffer.  But this feeble attempt didn't seem to  
> help:

That's unrelated issue. A pseudo file object might not have .buffer, but a
real stdio object should have one.

Perhaps, the underlying file descriptor would be messed up in Windows way.
If we can know that prior to calling write(), stdio fds can be reattached to
NUL. But I don't know if that's possible.

Another Py3 issue is that sys.stdin/stdout/stderr is None on pythonw.exe.
IIRC, this can be worked around by attaching NUL to fd=0-2, and open them.

  nullfd = os.open(os.devnull, O_RDWR | O_BINARY)
  os.dup2(nullfd, 0)
  os.dup2(nullfd, 1)
  os.dup2(nullfd, 2)
  os.close(nullfd)
  stdin = os.fdopen(0, 'rb')
  stdout = os.fdopen(1, 'wb')
  stderr = os.fdopen(2, 'wb')

But it might break ui.isatty() since NUL is reported as a tty on Windows.
Matt Harbison - Dec. 16, 2018, 4:58 a.m.
On Mon, 10 Dec 2018 07:12:41 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 09 Dec 2018 23:39:33 -0500, Matt Harbison wrote:
>> I ended up with similar stdio errors trying to track down the bad http
>> status failures:
>>
>> Traceback (most recent call last):
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1033, in _writenobuf
>>      dest.flush()
>> OSError: [WinError 1] Incorrect function
>>
>> During handling of the above exception, another exception occurred:
>>
>> Traceback (most recent call last):
>>    File "c:\Users\Matt\hg\mercurial\hgweb\server.py", line 192, in  
>> do_hgweb
>>      for chunk in self.server.application(env, self._start_response):
>>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 310, in
>> run_wsgi
>>      for r in self._runwsgi(req, res, repo):
>>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 430, in
>> _runwsgi
>>      return getattr(webcommands, cmd)(rctx)
>>    File "c:\Users\Matt\hg\mercurial\hgweb\webcommands.py", line 861, in
>> comparison
>>      context = parsecontext(web.config('web', 'comparisoncontext', '5'))
>>    File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 117, in  
>> config
>>      untrusted=untrusted)
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 517, in config
>>      untrusted=untrusted)
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 536, in _config
>>      self.develwarn(msg, 2, 'warn-config-unknown')
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1790, in develwarn
>>      % (msg, fname, lineno, fmsg))
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 996, in write_err
>>      self._write(self._ferr, *args, **opts)
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1006, in _write
>>      self._writenobuf(dest, *args, **opts)
>>    File "c:\Users\Matt\hg\mercurial\ui.py", line 1039, in _writenobuf
>>      raise error.StdioError(err)
>> mercurial.error.StdioError: [Errno 22] Incorrect function
>>
>> I see that there's a TODO for pycompat.{stdin,stdout,stderr} to use a
>> silly wrapper instead of .buffer.  But this feeble attempt didn't seem  
>> to
>> help:
>
> That's unrelated issue. A pseudo file object might not have .buffer, but  
> a
> real stdio object should have one.
>
> Perhaps, the underlying file descriptor would be messed up in Windows  
> way.
> If we can know that prior to calling write(), stdio fds can be  
> reattached to
> NUL. But I don't know if that's possible.

I'm not sure what to do with this info yet, but I just noticed that pager  
is also messed up on py3- the debug message about spawning the pager  
prints, but no output for the diff.  Use --pager=no, and it shows up.  So  
maybe there's a general problem with stdio of spawned children.

I've been wondering why we aren't spawning the server child process with  
stdout and stderr handles set to NUL.  I know on Unix, the child prints a  
bit of info before it starts listening.  But that never worked on Windows,  
and we've been abusing the lock file to convey error messages back to the  
parent.

> Another Py3 issue is that sys.stdin/stdout/stderr is None on pythonw.exe.
> IIRC, this can be worked around by attaching NUL to fd=0-2, and open  
> them.
>
>   nullfd = os.open(os.devnull, O_RDWR | O_BINARY)
>   os.dup2(nullfd, 0)
>   os.dup2(nullfd, 1)
>   os.dup2(nullfd, 2)
>   os.close(nullfd)
>   stdin = os.fdopen(0, 'rb')
>   stdout = os.fdopen(1, 'wb')
>   stderr = os.fdopen(2, 'wb')
>
> But it might break ui.isatty() since NUL is reported as a tty on Windows.


About NUL being a tty, maybe this will help?

	https://stackoverflow.com/a/3660125

Patch

diff -r 8c34826326a9 mercurial/server.py
--- a/mercurial/server.py      Sun Dec 09 13:53:08 2018 -0500
+++ b/mercurial/server.py      Sun Dec 09 22:58:06 2018 -0500
@@ -27,6 +27,16 @@ 

  def runservice(opts, parentfn=None, initfn=None, runfn=None, logfile=None,
                 runargs=None, appendpid=False):
+    try:
+        return _runservice(opts, parentfn, initfn, runfn, logfile,  
runargs, appendpid)
+    except: # Exception as e:
+        with open(r'C:\\Users\\Matt\\hg\\crash_%d' % os.getpid(), 'wb')  
as fp:
+            import traceback
+            s = traceback.format_exc()
+            fp.write(pycompat.sysbytes(s))
+
+def _runservice(opts, parentfn=None, initfn=None, runfn=None,  
logfile=None,
+               runargs=None, appendpid=False):
      '''Run a command as a service.'''

      postexecargs = {}

That yielded this in the logs when running test-pull-bundle.t:

$ cat ../crash_21284 ../crash_7252
Traceback (most recent call last):
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1029, in _writenobuf
     dest.write(msg)
   File "c:\Users\Matt\hg\mercurial\windows.py", line 198, in write
     self.fp.write(s[start:end])
   File "c:\Users\Matt\hg\mercurial\pycompat.py", line 132, in write
     return self._stream.write(encoding.strfromlocal(b))
OSError: [WinError 6] The handle is invalid

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
   File "c:\Users\Matt\hg\mercurial\server.py", line 31, in runservice
     return _runservice(opts, parentfn, initfn, runfn, logfile, runargs,  
appendpid)
   File "c:\Users\Matt\hg\mercurial\server.py", line 119, in _runservice
     initfn()
   File "c:\Users\Matt\hg\mercurial\hgweb\__init__.py", line 94, in init
     (url, pycompat.sysbytes(bindaddr), self.httpd.port))
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1493, in status
     self._writemsg(self._fmsgout, type='status', *msg, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1045, in _writemsg
     _writemsgwith(self._write, dest, *args, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 2039, in _writemsgwith
     write(dest, *args, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1006, in _write
     self._writenobuf(dest, *args, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1039, in _writenobuf
     raise error.StdioError(err)
mercurial.error.StdioError: [Errno 9] The handle is invalid
Traceback (most recent call last):
   File "c:\Users\Matt\hg\mercurial\server.py", line 31, in runservice
     return _runservice(opts, parentfn, initfn, runfn, logfile, runargs,  
appendpid)
   File "c:\Users\Matt\hg\mercurial\server.py", line 109, in _runservice
     raise error.Abort(_('child process failed to start'))
mercurial.error.Abort: b'child process failed to start'


I ended up with similar stdio errors trying to track down the bad http  
status failures:

Traceback (most recent call last):
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1033, in _writenobuf
     dest.flush()
OSError: [WinError 1] Incorrect function

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
   File "c:\Users\Matt\hg\mercurial\hgweb\server.py", line 192, in do_hgweb
     for chunk in self.server.application(env, self._start_response):
   File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 310, in  
run_wsgi
     for r in self._runwsgi(req, res, repo):
   File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 430, in  
_runwsgi
     return getattr(webcommands, cmd)(rctx)
   File "c:\Users\Matt\hg\mercurial\hgweb\webcommands.py", line 861, in  
comparison
     context = parsecontext(web.config('web', 'comparisoncontext', '5'))
   File "c:\Users\Matt\hg\mercurial\hgweb\hgweb_mod.py", line 117, in config
     untrusted=untrusted)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 517, in config
     untrusted=untrusted)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 536, in _config
     self.develwarn(msg, 2, 'warn-config-unknown')
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1790, in develwarn
     % (msg, fname, lineno, fmsg))
   File "c:\Users\Matt\hg\mercurial\ui.py", line 996, in write_err
     self._write(self._ferr, *args, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1006, in _write
     self._writenobuf(dest, *args, **opts)
   File "c:\Users\Matt\hg\mercurial\ui.py", line 1039, in _writenobuf
     raise error.StdioError(err)
mercurial.error.StdioError: [Errno 22] Incorrect function


I see that there's a TODO for pycompat.{stdin,stdout,stderr} to use a  
silly wrapper instead of .buffer.  But this feeble attempt didn't seem to  
help:

diff -r 8c34826326a9 -r 8e7e7cb35755 mercurial/pycompat.py
--- a/mercurial/pycompat.py    Sun Dec 09 13:53:08 2018 -0500
+++ b/mercurial/pycompat.py    Sun Dec 09 15:03:44 2018 -0500
@@ -120,11 +120,29 @@ 

      long = int

+    class sillywrapper(object):
+        def __init__(self, stream):
+            self._stream = stream
+            self.flush = self._stream.flush
+            self.close = self._stream.close
+            self.fileno = self._stream.fileno
+        def write(self, b):
+            from mercurial import encoding
+            # TODO: needs bytes written, not chars
+            return self._stream.write(encoding.strfromlocal(b))
+        def writelines(self, blines):
+            from mercurial import encoding
+            # TODO: needs bytes written, not chars
+            return self._stream.writelines([encoding.strfromlocal(b) for  
b in blines])
+
      # 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
-    stdout = sys.stdout.buffer
-    stderr = sys.stderr.buffer
+#    stdout = os.fdopen(sys.stdout.fileno(), r'wb')
+#    stderr = os.fdopen(sys.stderr.fileno(), r'wb')
+    stdout = sillywrapper(sys.stdout)
+    stderr = sillywrapper(sys.stderr)
+