Enable Py36+libuv+Appveyor

All tests are passing locally after fixing a GC-cycle related issue in
the io watcher that kept the wrong socket watchers alive. This should
probably be generalized to all watchers on all FFI platforms.

appveyor.yml

@@ -21,6 +21,12 @@ environment:
       PYTHON_ARCH: "64"
       PYTHON_EXE: python
 
+    - PYTHON: "C:\\Python36-x64"
+      PYTHON_VERSION: "3.6.x" # currently 3.6.0
+      PYTHON_ARCH: "64"
+      PYTHON_EXE: python
+      GEVENT_CORE_CFFI_ONLY: libuv
+
     - PYTHON: "C:\\Python27-x64"
       PYTHON_VERSION: "2.7.x" # currently 2.7.13
       PYTHON_ARCH: "64"
@@ -149,6 +155,7 @@ build_script:
 
 test_script:
   # Run the project tests
+  - "%PYEXE% -c \"import gevent.core; print(gevent.core.loop)\""
   - "cd src/greentest && %PYEXE% testrunner.py --config known_failures.py --quiet && cd ../.."
 
 after_test:

examples/concurrent_download.py

@@ -11,7 +11,13 @@ monkey.patch_all()
 
 import sys
 
-urls = ['http://www.google.com', 'http://www.yandex.ru', 'http://www.python.org']
+# Note that all of these redirect to HTTPS, so
+# this demonstrates that SSL works.
+urls = [
+    'http://www.google.com',
+    'http://www.apple.com',
+    'http://www.python.org'
+]
 
 
 if sys.version_info[0] == 3:

src/gevent/_ffi/watcher.py

@@ -151,7 +151,7 @@ class watcher(object):
 
     _callback = None
     _args = None
-    _handle = None # FFI object to self
+    _handle = None # FFI object to self. This is a GC cycle. See _watcher_create
     _watcher = None
 
     def __init__(self, _loop, ref=True, priority=None, args=_NOARGS):
@@ -178,7 +178,11 @@ class watcher(object):
         pass
 
     def _watcher_create(self, ref): # pylint:disable=unused-argument
-        self._handle = type(self).new_handle(self) # This is a GC cycle pylint:disable=no-member
+        # self._handle has a reference to self, keeping it alive.
+        # We must keep self._handle alive for ffi.from_handle() to be
+        # able to work.
+        # THIS IS A GC CYCLE.
+        self._handle = type(self).new_handle(self) # pylint:disable=no-member
         self._watcher = self._watcher_new()
         # This call takes care of calling _watcher_ffi_close when
         # self goes away, making sure self._watcher stays alive
@@ -346,8 +350,8 @@ class IoMixin(object):
     EVENT_MASK = 0
 
     def __init__(self, loop, fd, events, ref=True, priority=None, _args=None):
-        # XXX: Win32: Need to vfd_open the fd and free the old one?
-        # XXX: Win32: Need a destructor to free the old fd?
+        # Win32 only works with sockets, and only when we use libuv, because
+        # we don't use _open_osfhandle. See libuv/watchers.py:io for a description.
         if fd < 0:
             raise ValueError('fd must be non-negative: %r' % fd)
         if events & ~self.EVENT_MASK:

src/gevent/libuv/_corecffi_build.py

@@ -49,7 +49,8 @@ _cdef = _cdef.replace("GEVENT_STRUCT_DONE _;", '...;')
 # just another name for handle, which is just another name for 'void*'
 # which we will treat as an 'unsigned long' or 'unsigned long long'
 # since it comes through 'fileno()' where it has been cast as an int.
-_void_pointer_as_integer = 'unsigned long' if system_bits() == 32 else 'unsigned long long'
+# See class watcher.io
+_void_pointer_as_integer = 'intptr_t' #'unsigned long' if system_bits() == 32 else 'unsigned long long'
 _cdef = _cdef.replace("GEVENT_UV_OS_SOCK_T", 'int' if not WIN else _void_pointer_as_integer)
 
 

src/gevent/libuv/loop.py

@@ -10,7 +10,8 @@ from collections import namedtuple
 import signal
 from weakref import WeakValueDictionary
 
-
+from gevent._compat import PYPY
+from gevent._compat import WIN
 from gevent._ffi.loop import AbstractLoop
 from gevent.libuv import _corecffi # pylint:disable=no-name-in-module,import-error
 from gevent._ffi.loop import assign_standard_callbacks
@@ -49,6 +50,20 @@ def get_header_version():
 def supported_backends():
     return ['default']
 
+if PYPY and WIN:
+    def gcsOnPyPy(f):
+        import functools
+        import gc
+
+        @functools.wraps(f)
+        def m(self, *args):
+            gc.collect()
+            return f(self, *args)
+        return m
+else:
+    def gcsOnPyPy(f):
+        return f
+
 class loop(AbstractLoop):
 
     DEFAULT_LOOP_REGENERATES = True
@@ -335,12 +350,17 @@ class loop(AbstractLoop):
                 watcher._set_status(status)
 
 
+    @gcsOnPyPy
     def io(self, fd, events, ref=True, priority=None):
         # We don't keep a hard ref to the root object;
-        # the caller should keep the multiplexed watcher
+        # the caller must keep the multiplexed watcher
         # alive as long as its in use.
-        # XXX: Note there is a cycle from io_watcher._handle -> io_watcher
-        # so these aren't collected as soon as you think/hope.
+
+        # We go to great pains to avoid GC cycles here, otherwise
+        # CPython tests (e.g., test_asyncore) fail on Windows.
+        # For PyPy, though, avoiding cycles isn't enough and we must
+        # do a GC to force cleaning up old objects.
+
         io_watchers = self._io_watchers
         try:
             io_watcher = io_watchers[fd]

src/gevent/libuv/watcher.py

@@ -31,7 +31,7 @@ def _pid_dbg(*args, **kwargs):
     kwargs['file'] = sys.stderr
     print(os.getpid(), *args, **kwargs)
 
-#_dbg = _pid_dbg
+# _dbg = _pid_dbg
 
 _events = [(libuv.UV_READABLE, "READ"),
            (libuv.UV_WRITABLE, "WRITE")]
@@ -198,12 +198,31 @@ class io(_base.IoMixin, watcher):
 
     EVENT_MASK = libuv.UV_READABLE | libuv.UV_WRITABLE | libuv.UV_DISCONNECT
 
-    _multiplex_watchers = None
-
     def __init__(self, loop, fd, events, ref=True, priority=None):
         super(io, self).__init__(loop, fd, events, ref=ref, priority=priority, _args=(fd,))
         self._fd = fd
         self._events = events
+        self._multiplex_watchers = []
+
+    def _watcher_create(self, ref):
+        super(io, self)._watcher_create(ref)
+        # Immediately break the GC cycle. We restore the cycle before
+        # we are started and break it again when we are stopped.
+
+        # On Windows is critical to be able to garbage collect these
+        # objects in a timely fashion so that they don't get reused
+        # for multiplexing completely different sockets. This is because
+        # uv_poll_init_socket does a lot of setup for the socket to make
+        # polling work. If get reused for another socket that has the same
+        # fileno, things break badly. (In theory this could be a problem
+        # on posix too, but in practice it isn't).
+
+        # TODO: We should probably generalize this to all
+        # ffi watchers. Avoiding GC cycles as much as possible
+        # is a good thing, and potentially allocating new handles
+        # as needed gets us better memory locality.
+        self._handle = None
+        self._watcher.data = ffi.NULL
 
     def _get_fd(self):
         return self._fd
@@ -221,11 +240,44 @@ class io(_base.IoMixin, watcher):
         self._events = events
 
     def _watcher_ffi_start(self):
+        assert self._handle is None
+        self._handle = self._watcher.data = type(self).new_handle(self)
         self._watcher_start(self._watcher, self._events, self._watcher_callback)
 
+    def _watcher_ffi_stop(self):
+        # We may or may not have been started yet, so
+        # we may or may not have a handle; either way,
+        # drop it.
+        self._handle = None
+        self._watcher.data = ffi.NULL
+        super(io, self)._watcher_ffi_stop()
+
     if sys.platform.startswith('win32'):
-        # We can only handle sockets. We smuggle the SOCKET through disguised
-        # as a fileno
+        # uv_poll can only handle sockets on Windows, but the plain
+        # uv_poll_init we call on POSIX assumes that the fileno
+        # argument is already a C fileno, as created by
+        # _get_osfhandle. C filenos are limited resources, must be
+        # closed with _close. So there are lifetime issues with that:
+        # calling the C function _close to dispose of the fileno
+        # *also* closes the underlying win32 handle, possibly
+        # prematurely. (XXX: Maybe could do something with weak
+        # references? But to what?)
+
+        # All libuv wants to do with the fileno in uv_poll_init is
+        # turn it back into a Win32 SOCKET handle.
+
+        # Now, libuv provides uv_poll_init_socket, which instead of
+        # taking a C fileno takes the SOCKET, avoiding the need to dance with
+        # the C runtime.
+
+        # It turns out that SOCKET (win32 handles in general) can be
+        # represented with `intptr_t`. It further turns out that
+        # CPython *directly* exposes the SOCKET handle as the value of
+        # fileno (32-bit PyPy does some munging on it, which should
+        # rarely matter). So we can pass socket.fileno() through
+        # to uv_poll_init_socket.
+
+        # See _corecffi_build.
         _watcher_init = watcher._LIB.uv_poll_init_socket
 
 
@@ -243,12 +295,14 @@ class io(_base.IoMixin, watcher):
             # These objects keep the original IO object alive;
             # the IO object SHOULD NOT keep these alive to avoid cycles
             # When they all go away, the original IO object can go
-            # away. Hopefully that means that the FD they were opened for
+            # away. *Hopefully* that means that the FD they were opened for
             # has also gone away.
             self._watcher_ref = watcher
 
         def start(self, callback, *args, **kwargs):
-            _dbg("Starting IO multiplex watcher for", self.fd, callback)
+            _dbg("Starting IO multiplex watcher for", self.fd,
+                 "callback", callback,
+                 "owner", self._watcher_ref)
             self.pass_events = kwargs.get("pass_events")
             self.callback = callback
             self.args = args
@@ -258,7 +312,9 @@ class io(_base.IoMixin, watcher):
                 watcher._io_start()
 
         def stop(self):
-            _dbg("Stopping IO multiplex watcher for", self.fd, self.callback)
+            _dbg("Stopping IO multiplex watcher for", self.fd,
+                 "callback", self.callback,
+                 "owner", self._watcher_ref)
             self.callback = None
             self.pass_events = None
             self.args = None
@@ -282,10 +338,10 @@ class io(_base.IoMixin, watcher):
     def _io_maybe_stop(self):
         for r in self._multiplex_watchers:
             w = r()
-            if w is None:
+            if w is not None and w.callback is not None:
+                # There's still a reference to it, and it's started,
+                # so we can't stop.
                 continue
-            if w.callback is not None:
-                return
         # If we get here, nothing was started
         # so we can take ourself out of the polling set
         self.stop()

src/gevent/select.py

@@ -16,13 +16,17 @@ from gevent._util import copy_globals
 from gevent._util import _NONE
 
 from errno import EINTR
+from select import select as _real_original_select
 if sys.platform.startswith('win32'):
-    def _original_select(_r, _w, _x, _t):
+    def _original_select(r, w, x, t):
         # windows cant handle three empty lists, but we've always
-        # accepted that, so don't try the compliance check on windows
-        return ((), (), ())
+        # accepted that
+        if not r and not w and not x:
+            return ((), (), ())
+        return _real_original_select(r, w, x, t)
 else:
-    from select import select as _original_select
+    _original_select = _real_original_select
+
 
 try:
     from select import poll as original_poll

src/greentest/patched_tests_setup.py

@@ -17,6 +17,7 @@ TRAVIS = os.environ.get("TRAVIS") == "true"
 OSX = sys.platform == 'darwin'
 PYPY = hasattr(sys, 'pypy_version_info')
 WIN = sys.platform.startswith("win")
+PY3 = sys.version_info[0] >= 3
 
 # XXX: Formalize this better
 LIBUV = os.getenv('GEVENT_CORE_CFFI_ONLY') == 'libuv' or (PYPY and WIN)
@@ -293,6 +294,19 @@ if LIBUV:
             'test_urllib2_localnet.TestUrlopen.test_https_with_cafile',
         ]
 
+    if WIN:
+
+        disabled_tests += [
+            # This test winds up hanging a long time.
+            # Inserting GCs doesn't fix it.
+            'test_ssl.ThreadedTests.test_handshake_timeout',
+        ]
+
+        if PY3:
+
+            disabled_tests += [
+            ]
+
 def _make_run_with_original(mod_name, func_name):
     @contextlib.contextmanager
     def with_orig():

src/greentest/test___monkey_patching.py

@@ -44,7 +44,7 @@ def TESTRUNNER(tests=None):
                'timeout': TIMEOUT,
                'setenv': {'PYTHONPATH': PYTHONPATH}}
 
-    if tests:
+    if tests and not sys.platform.startswith("win"):
         atexit.register(os.system, 'rm -f */@test*')
 
     basic_args = [sys.executable, '-u', '-W', 'ignore', '-m' 'monkey_test']

src/greentest/test__issue600.py

@@ -5,6 +5,7 @@ import gevent
 from gevent import monkey
 monkey.patch_all()
 
+import sys
 from multiprocessing import Process
 from gevent.subprocess import Popen, PIPE
 
@@ -14,7 +15,7 @@ def test_invoke():
     # libev is handling SIGCHLD. This could *probably* be simplified to use
     # just hub.loop.install_sigchld
 
-    p = Popen("true", stdout=PIPE, stderr=PIPE)
+    p = Popen([sys.executable, '-V'], stdout=PIPE, stderr=PIPE)
     gevent.sleep(0)
     p.communicate()
     gevent.sleep(0)