Merge pull request #1056 from gevent/libuv-py36-win

Support libuv under CPython 3.6 on Windows

.pylintrc

@@ -29,6 +29,12 @@
 # useless-suppression: the only way to avoid repeating it for specific statements everywhere that we
 #   do Py2/Py3 stuff is to put it here. Sadly this means that we might get better but not realize it.
 # duplicate-code: Yeah, the compatibility ssl modules are much the same
+# In pylint 1.8.0, inconsistent-return-statements are created for the wrong reasons.
+# This code raises it, even though there's only one return (the implicit 'return None' is presumably
+# what triggers it):
+#  def foo():
+#    if baz:
+#      return 1
 disable=wrong-import-position,
     wrong-import-order,
     missing-docstring,
@@ -44,9 +50,10 @@ disable=wrong-import-position,
     cyclic-import,
     too-many-arguments,
     redefined-builtin,
-	useless-suppression,
+    useless-suppression,
     duplicate-code,
-	undefined-all-variable
+    undefined-all-variable,
+    inconsistent-return-statements
 
 
 [FORMAT]

appveyor.yml

@@ -10,16 +10,24 @@ environment:
     # Pre-installed Python versions, which Appveyor may upgrade to
     # a later point release.
 
-    - PYTHON: "C:\\pypy2-v5.9.0-win32"
-      PYTHON_ID: "pypy"
-      PYTHON_EXE: pypy
-      PYTHON_VERSION: "2.7.x"
-      PYTHON_ARCH: "32"
+    # We're not quite ready for PyPy+libuv, it doesn't even
+    # work correctly on Posix.
+    # - PYTHON: "C:\\pypy2-v5.9.0-win32"
+    #   PYTHON_ID: "pypy"
+    #   PYTHON_EXE: pypy
+    #   PYTHON_VERSION: "2.7.x"
+    #   PYTHON_ARCH: "32"
+
+    - PYTHON: "C:\\Python36-x64"
+      PYTHON_VERSION: "3.6.x" # currently 3.6.0
+      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
@@ -149,6 +157,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:

dev-requirements.txt

@@ -2,7 +2,7 @@ setuptools
 wheel
 cython>=0.27.3
 greenlet>=0.4.10
-pylint>=1.7.1
+pylint>=1.8.0
 prospector[with_pyroma]
 coverage>=4.0
 coveralls>=1.0

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/__init__.py

 """
 Internal helpers for FFI implementations.
 """
+from __future__ import print_function, absolute_import
+
+import os
+import sys
+
+def _dbg(*args, **kwargs):
+    # pylint:disable=unused-argument
+    pass
+
+#_dbg = print
+
+def _pid_dbg(*args, **kwargs):
+    kwargs['file'] = sys.stderr
+    print(os.getpid(), *args, **kwargs)
+
+GEVENT_DEBUG = 0
+CRITICAL = 1
+ERROR = 3
+DEBUG = 5
+TRACE = 9
+
+if os.getenv("GEVENT_DEBUG") == 'critical':
+    GEVENT_DEBUG = CRITICAL
+elif os.getenv("GEVENT_DEBUG") == 'error':
+    GEVENT_DEBUG = ERROR
+elif os.getenv('GEVENT_DEBUG') == 'debug':
+    GEVENT_DEBUG = DEBUG
+elif os.getenv('GEVENT_DEBUG') == 'trace':
+    _dbg = _pid_dbg
+    GEVENT_DEBUG = TRACE

src/gevent/_ffi/loop.py

@@ -8,6 +8,10 @@ import os
 import traceback
 from weakref import ref as WeakRef
 
+from gevent._ffi import _dbg
+from gevent._ffi import GEVENT_DEBUG
+from gevent._ffi import TRACE
+from gevent._ffi import CRITICAL
 from gevent._ffi.callback import callback
 
 __all__ = [
@@ -59,9 +63,18 @@ EVENTS = GEVENT_CORE_EVENTS = _EVENTSType()
 ####
 class _Callbacks(object):
 
+
     def __init__(self, ffi):
         self.ffi = ffi
         self.callbacks = []
+        if GEVENT_DEBUG < TRACE:
+            self.from_handle = ffi.from_handle
+
+    def from_handle(self, handle): # pylint:disable=method-hidden
+        _dbg("Getting from handle", handle)
+        x = self.ffi.from_handle(handle)
+        _dbg("Got from handle", handle, x)
+        return x
 
     def python_callback(self, handle, revents):
         """
@@ -71,21 +84,35 @@ class _Callbacks(object):
           An exception occurred during the callback and you must call
           :func:`_python_handle_error` to deal with it. The Python watcher
           object will have the exception tuple saved in ``_exc_info``.
-        - 0
-          Everything went according to plan. You should check to see if the libev
-          watcher is still active, and call :func:`_python_stop` if so. This will
-          clean up the memory.
         - 1
+          Everything went according to plan. You should check to see if the libev
+          watcher is still active, and call :func:`python_stop` if it is not. This will
+          clean up the memory. Finding the watcher still active at the event loop level,
+          but not having stopped itself at the gevent level is a buggy scenario and
+          shouldn't happen.
+        - 2
           Everything went according to plan, but the watcher has already
           been stopped. Its memory may no longer be valid.
+
+        This function should never return 0, as that's the default value that
+        Python exceptions will produce.
         """
+        orig_ffi_watcher = None
         try:
             # Even dereferencing the handle needs to be inside the try/except;
             # if we don't return normally (e.g., a signal) then we wind up going
-            # to the 'onerror' handler, which
+            # to the 'onerror' handler (unhandled_onerror), which
             # is not what we want; that can permanently wedge the loop depending
-            # on which callback was executing
-            the_watcher = self.ffi.from_handle(handle)
+            # on which callback was executing.
+            # XXX: See comments in that function. We may be able to restart and do better?
+            if not handle:
+                # Hmm, a NULL handle. That's not supposed to happen.
+                # We can easily get into a loop if we deref it and allow that
+                # to raise.
+                _dbg("python_callback got null handle")
+                return 1
+            the_watcher = self.from_handle(handle)
+            orig_ffi_watcher = the_watcher._watcher
             args = the_watcher.args
             if args is None:
                 # Legacy behaviour from corecext: convert None into ()
@@ -95,13 +122,14 @@ class _Callbacks(object):
                 args = (revents, ) + args[1:]
             the_watcher.callback(*args)
         except: # pylint:disable=bare-except
+            _dbg("Got exception servicing watcher with handle", handle)
             # It's possible for ``the_watcher`` to be undefined (UnboundLocalError)
-            # if we threw an exception on the line that created that variable.
+            # if we threw an exception (signal) on the line that created that variable.
             # This is typically the case with a signal under libuv
             try:
                 the_watcher
             except UnboundLocalError:
-                the_watcher = self.ffi.from_handle(handle)
+                the_watcher = self.from_handle(handle)
             the_watcher._exc_info = sys.exc_info()
             # Depending on when the exception happened, the watcher
             # may or may not have been stopped. We need to make sure its
@@ -109,14 +137,23 @@ class _Callbacks(object):
             the_watcher.loop._keepaliveset.add(the_watcher)
             return -1
         else:
-            if the_watcher in the_watcher.loop._keepaliveset:
-                # It didn't stop itself
-                return 0
-            return 1 # It stopped itself
+            if the_watcher in the_watcher.loop._keepaliveset and the_watcher._watcher is orig_ffi_watcher:
+                # It didn't stop itself, *and* it didn't stop itself, reset
+                # its watcher, and start itself again. libuv's io watchers MAY
+                # do that.
+                # The normal, expected scenario when we find the watcher still
+                # in the keepaliveset is that it is still active at the event loop
+                # level, so we don't expect that python_stop gets called.
+                _dbg("The watcher has not stopped itself, possibly still active", the_watcher)
+                return 1
+            return 2 # it stopped itself
 
     def python_handle_error(self, handle, _revents):
+        _dbg("Handling error for handle", handle)
+        if not handle:
+            return
         try:
-            watcher = self.ffi.from_handle(handle)
+            watcher = self.from_handle(handle)
             exc_info = watcher._exc_info
             del watcher._exc_info
             # In the past, we passed the ``watcher`` itself as the context,
@@ -145,15 +182,45 @@ class _Callbacks(object):
 
     def unhandled_onerror(self, t, v, tb):
         # This is supposed to be called for signals, etc.
+        # This is the onerror= value for CFFI.
+        # If we return None, C will get a value of 0/NULL;
+        # if we raise, CFFI will print the exception and then
+        # return 0/NULL; (unless error= was configured)
+        # If things go as planned, we return the value that asks
+        # C to call back and check on if the watcher needs to be closed or
+        # not.
+
+        # XXX: TODO: Could this cause events to be lost? Maybe we need to return
+        # a value that causes the C loop to try the callback again?
+        # at least for signals under libuv, which are delivered at very odd times.
+        # Hopefully the event still shows up when we poll the next time.
+
+        watcher = None
         if tb is not None:
             handle = tb.tb_frame.f_locals['handle']
-            watcher = self.ffi.from_handle(handle)
+            if handle: # handle could be NULL
+                watcher = self.from_handle(handle)
+        if watcher is not None:
             watcher.loop._check_callback_handle_error(t, v, tb)
+            return 1
         else:
             raise v
 
     def python_stop(self, handle):
-        watcher = self.ffi.from_handle(handle)
+        if not handle:
+            print(
+                "WARNING: gevent: Unable to dereference handle; not stopping watcher. "
+                "Native resources may leak. This is most likely a bug in gevent.",
+                file=sys.stderr)
+            # The alternative is to crash with no helpful information
+            # NOTE: Raising exceptions here does nothing, they're swallowed by CFFI.
+            # Since the C level passed in a null pointer, even dereferencing the handle
+            # will just produce some exceptions.
+            if GEVENT_DEBUG < CRITICAL:
+                return
+            import pdb; pdb.set_trace()
+        _dbg("python_stop: stopping watcher with handle", handle)
+        watcher = self.from_handle(handle)
         watcher.stop()
 
 
@@ -490,7 +557,7 @@ class AbstractLoop(object):
     def fork(self, ref=True, priority=None):
         return self._watchers.fork(self, ref, priority)
 
-    def async(self, ref=True, priority=None):
+    def async(self, ref=True, priority=None): # XXX: Yeah, we know. pylint:disable=assign-to-new-keyword
         return self._watchers.async(self, ref, priority)
 
     if sys.platform != "win32":

src/gevent/_ffi/watcher.py

@@ -9,6 +9,7 @@ import os
 import signal as signalmodule
 import functools
 
+from gevent._ffi import _dbg
 from gevent._ffi.loop import GEVENT_CORE_EVENTS
 from gevent._ffi.loop import _NOARGS
 
@@ -16,6 +17,12 @@ __all__ = [
 
 ]
 
+class _NoWatcherResult(int):
+
+    def __repr__(self):
+        return "<NoWatcher>"
+
+_NoWatcherResult = _NoWatcherResult(0)
 
 def events_to_str(event_field, all_events):
     result = []
@@ -35,10 +42,25 @@ def not_while_active(func):
     @functools.wraps(func)
     def nw(self, *args, **kwargs):
         if self.active:
-            raise AttributeError("not while active")
+            raise ValueError("not while active")
         func(self, *args, **kwargs)
     return nw
 
+def only_if_watcher(func):
+    @functools.wraps(func)
+    def if_w(self):
+        if self._watcher:
+            return func(self)
+        return _NoWatcherResult
+    return if_w
+
+def error_if_no_watcher(func):
+    @functools.wraps(func)
+    def no_w(self):
+        if not self._watcher:
+            raise ValueError("No watcher present", self)
+        func(self)
+    return no_w
 
 class LazyOnClass(object):
 
@@ -151,11 +173,29 @@ 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
 
+    # Do we create the native resources when this class is created?
+    # If so, we call _watcher_full_init from the constructor.
+    # Otherwise, it must be called before we are started.
+    # If a subclass sets this to false, they must make that call.
+    # Currently unused. Experimental functionality for libuv.
+    _watcher_init_on_init = True
+
     def __init__(self, _loop, ref=True, priority=None, args=_NOARGS):
         self.loop = _loop
+        self.__init_priority = priority
+        self.__init_args = args
+        self.__init_ref = ref
+
+        if self._watcher_init_on_init:
+            self._watcher_full_init()
+
+    def _watcher_full_init(self):
+        priority = self.__init_priority
+        ref = self.__init_ref
+        args = self.__init_args
 
         self._watcher_create(ref)
 
@@ -178,7 +218,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
@@ -262,13 +306,15 @@ class watcher(object):
             result += " args=%r" % (self.args, )
         if self.callback is None and self.args is None:
             result += " stopped"
+        result += " watcher=%s" % (self._watcher)
         result += " handle=%s" % (self._watcher_handle)
         result += " ref=%s" % (self.ref)
         return result + ">"
 
     @property
     def _watcher_handle(self):
-        return self._watcher.data
+        if self._watcher:
+            return self._watcher.data
 
     def _format(self):
         return ''
@@ -314,10 +360,11 @@ class watcher(object):
         self._watcher_ffi_start_unref()
 
     def stop(self):
+        _dbg("Main stop for", self, "keepalive?", self in self.loop._keepaliveset)
         self._watcher_ffi_stop_ref()
         self._watcher_ffi_stop()
         self.loop._keepaliveset.discard(self)
-
+        _dbg("Finished main stop for", self, "keepalive?", self in self.loop._keepaliveset)
         self.callback = None
         self.args = None
 
@@ -333,7 +380,9 @@ class watcher(object):
 
     @property
     def active(self):
-        return True if self._watcher_is_active(self._watcher) else False
+        if self._watcher is not None and self._watcher_is_active(self._watcher):
+            return True
+        return False
 
     @property
     def pending(self):
@@ -346,8 +395,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/_socket2.py

@@ -202,13 +202,22 @@ class socket(object):
 
     def close(self, _closedsocket=_closedsocket, cancel_wait_ex=cancel_wait_ex):
         # This function should not reference any globals. See Python issue #808164.
-        self.hub.cancel_wait(self._read_event, cancel_wait_ex)
-        self.hub.cancel_wait(self._write_event, cancel_wait_ex)
+
+        # Also break any reference to the loop.io objects. Our fileno, which they were
+        # tied to, is now free to be reused, so these objects are no longer functional.
+
+        if self._read_event is not None:
+            self.hub.cancel_wait(self._read_event, cancel_wait_ex)
+            self._read_event = None
+        if self._write_event is not None:
+            self.hub.cancel_wait(self._write_event, cancel_wait_ex)
+            self._write_event = None
         s = self._sock
         self._sock = _closedsocket()
         if PYPY:
             s._drop()
 
+
     @property
     def closed(self):
         return isinstance(self._sock, _closedsocket)
@@ -261,7 +270,10 @@ class socket(object):
     def makefile(self, mode='r', bufsize=-1):
         # Two things to look out for:
         # 1) Closing the original socket object should not close the
-        #    socket (hence creating a new instance)
+        #    socket (hence creating a new socket instance);
+        #    An alternate approach is what _socket3.py does, which is to
+        #    keep count of the times makefile objects have been opened (Py3's
+        #    SocketIO helps with that).
         # 2) The resulting fileobject must keep the timeout in order
         #    to be compatible with the stdlib's socket.makefile.
         # Pass self as _sock to preserve timeout.

src/gevent/_socket3.py

@@ -237,6 +237,7 @@ class socket(object):
         return text
 
     def _decref_socketios(self):
+        # Called by SocketIO when it is closed.
         if self._io_refs > 0:
             self._io_refs -= 1
         if self._closed:
@@ -244,8 +245,17 @@ class socket(object):
 
     def _real_close(self, _ss=_socket.socket, cancel_wait_ex=cancel_wait_ex):
         # This function should not reference any globals. See Python issue #808164.
-        self.hub.cancel_wait(self._read_event, cancel_wait_ex)
-        self.hub.cancel_wait(self._write_event, cancel_wait_ex)
+
+        # Break any reference to the loop.io objects. Our fileno,
+        # which they were tied to, is now free to be reused, so these
+        # objects are no longer functional.
+
+        if self._read_event is not None:
+            self.hub.cancel_wait(self._read_event, cancel_wait_ex)
+            self._read_event = None
+        if self._write_event is not None:
+            self.hub.cancel_wait(self._write_event, cancel_wait_ex)
+            self._write_event = None
         _ss.close(self._sock)
 
         # Break any references to the underlying socket object. Tested

src/gevent/libev/_corecffi_source.c

@@ -30,7 +30,7 @@ static void _gevent_generic_callback(struct ev_loop* loop,
             // and allowing memory to be freed
             python_handle_error(handle, revents);
         break;
-        case 0:
+        case 1:
             // Code to stop the event. Note that if python_callback
             // has disposed of the last reference to the handle,
             // `watcher` could now be invalid/disposed memory!
@@ -38,8 +38,16 @@ static void _gevent_generic_callback(struct ev_loop* loop,
                 python_stop(handle);
             }
         break;
-        default:
-            assert(cb_result == 1);
+        case 2:
             // watcher is already stopped and dead, nothing to do.
+        break;
+        default:
+            fprintf(stderr,
+                    "WARNING: gevent: Unexpected return value %d from Python callback "
+                    "for watcher %p and handle %d\n",
+                    cb_result,
+                    watcher, handle);
+            // XXX: Possible leaking of resources here? Should we be
+            // closing the watcher?
     }
 }

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'
 _cdef = _cdef.replace("GEVENT_UV_OS_SOCK_T", 'int' if not WIN else _void_pointer_as_integer)
 
 

src/gevent/libuv/_corecffi_source.c

@@ -10,9 +10,11 @@ static void (*gevent_noop)(void* handle) = &_gevent_noop;
 
 static void _gevent_generic_callback1(void* vwatcher, int arg)
 {
-	uv_handle_t* watcher = (uv_handle_t*)vwatcher;
+    const uv_handle_t* watcher = (uv_handle_t*)vwatcher;
+    // Python code may set this to NULL or even change it
+    // out from under us, which would tend to break things.
     void* handle = watcher->data;
-    int cb_result = python_callback(handle, arg);
+    const int cb_result = python_callback(handle, arg);
     switch(cb_result) {
         case -1:
             // in case of exception, call self.loop.handle_error;
@@ -20,62 +22,89 @@ static void _gevent_generic_callback1(void* vwatcher, int arg)
             // and allowing memory to be freed
             python_handle_error(handle, arg);
         break;
-        case 0:
-            // Code to stop the event. Note that if python_callback
+        case 1:
+            // Code to stop the event IF NEEDED. Note that if python_callback
             // has disposed of the last reference to the handle,
             // `watcher` could now be invalid/disposed memory!
             if (!uv_is_active(watcher)) {
-                python_stop(handle);
+                if (watcher->data != handle) {
+                    if (watcher->data) {
+                        // If Python set the data to NULL, then they
+                        // expected to be stopped. That's fine.
+                        // Otherwise, something weird happened.
+                        fprintf(stderr,
+                                "WARNING: gevent: watcher handle changed in callback "
+                                "from %p to %p for watcher at %p of type %d\n",
+                                handle, watcher->data, watcher, watcher->type);
+                        // There's a very good chance that the object the
+                        // handle referred to has been changed and/or the
+                        // old handle has been deallocated (most common), so
+                        // passing the old handle will crash. Instead we
+                        // pass a sigil to let python distinguish this case.
+                        python_stop(NULL);
+                    }
+                }
+                else {
+                    python_stop(handle);
+                }
             }
         break;
-        default:
-            assert(cb_result == 1);
+        case 2:
             // watcher is already stopped and dead, nothing to do.
+        break;
+        default:
+            fprintf(stderr,
+                    "WARNING: gevent: Unexpected return value %d from Python callback "
+                    "for watcher %p (of type %d) and handle %p\n",
+                    cb_result,
+                    watcher, watcher->type, handle);
+            // XXX: Possible leaking of resources here? Should we be
+            // closing the watcher?
     }
 }
 
 
 static void _gevent_generic_callback0(void* handle)
 {
-	_gevent_generic_callback1(handle, 0);
+    _gevent_generic_callback1(handle, 0);
 }
 
 static void _gevent_poll_callback2(void* handle, int status, int events)
 {
-	_gevent_generic_callback1(handle, status < 0 ? status : events);
+    _gevent_generic_callback1(handle, status < 0 ? status : events);
 }
 
 static void _gevent_fs_event_callback3(void* handle, const char* filename, int events, int status)
 {
-	_gevent_generic_callback1(handle, status < 0 ? status : events);
+    _gevent_generic_callback1(handle, status < 0 ? status : events);
 }
 
 
 typedef struct _gevent_fs_poll_s {
-	uv_fs_poll_t handle;
-	uv_stat_t curr;
-	uv_stat_t prev;
+    uv_fs_poll_t handle;
+    uv_stat_t curr;
+    uv_stat_t prev;
 } gevent_fs_poll_t;
 
 static void _gevent_fs_poll_callback3(void* handlep, int status, const uv_stat_t* prev, const uv_stat_t* curr)
 {
-	// stat pointers are valid for this callback only.
-	// if given, copy them into our structure, where they can be reached
-	// from python, just like libev's watcher does, before calling
-	// the callback.
+    // stat pointers are valid for this callback only.
+    // if given, copy them into our structure, where they can be reached
+    // from python, just like libev's watcher does, before calling
+    // the callback.
 
-	// The callback is invoked with status < 0 if path does not exist
-	// or is inaccessible. The watcher is not stopped but your
-	// callback is not called again until something changes (e.g. when
-	// the file is created or the error reason changes).
-	// In that case the fields will be 0 in curr/prev.
+    // The callback is invoked with status < 0 if path does not exist
+    // or is inaccessible. The watcher is not stopped but your
+    // callback is not called again until something changes (e.g. when
+    // the file is created or the error reason changes).
+    // In that case the fields will be 0 in curr/prev.
 
 
-	gevent_fs_poll_t* handle = (gevent_fs_poll_t*)handlep;
-	assert(status == 0);
+    gevent_fs_poll_t* handle = (gevent_fs_poll_t*)handlep;
+    assert(status == 0);
 
-	handle->curr = *curr;
-	handle->prev = *prev;
+    handle->curr = *curr;
+    handle->prev = *prev;
 
-	_gevent_generic_callback1((uv_handle_t*)handle, 0);
+    _gevent_generic_callback1((uv_handle_t*)handle, 0);
 }

src/gevent/libuv/loop.py

@@ -10,7 +10,7 @@ from collections import namedtuple
 import signal
 from weakref import WeakValueDictionary
 
-
+from gevent._compat import PYPY
 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 +49,20 @@ def get_header_version():
 def supported_backends():
     return ['default']
 
+if PYPY:
+    def gcBefore(f):
+        import functools
+        import gc
+
+        @functools.wraps(f)
+        def m(self, *args):
+            gc.collect()
+            return f(self, *args)
+        return m
+else:
+    def gcBefore(f):
+        return f
+
 class loop(AbstractLoop):
 
     DEFAULT_LOOP_REGENERATES = True
@@ -335,12 +349,17 @@ class loop(AbstractLoop):
                 watcher._set_status(status)
 
 
+    @gcBefore
     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

@@ -12,6 +12,7 @@ ffi = _corecffi.ffi
 libuv = _corecffi.lib
 
 from gevent._ffi import watcher as _base
+from gevent._ffi import _dbg
 
 _closing_handles = set()
 
@@ -20,18 +21,6 @@ _closing_handles = set()
 def _uv_close_callback(handle):
     _closing_handles.remove(handle)
 
-def _dbg(*args, **kwargs):
-    # pylint:disable=unused-argument
-    pass
-
-#_dbg = print
-
-def _pid_dbg(*args, **kwargs):
-    import os
-    kwargs['file'] = sys.stderr
-    print(os.getpid(), *args, **kwargs)
-
-#_dbg = _pid_dbg
 
 _events = [(libuv.UV_READABLE, "READ"),
            (libuv.UV_WRITABLE, "WRITE")]
@@ -157,14 +146,16 @@ class watcher(_base.watcher):
         _dbg("\tStarted", self)
 
     def _watcher_ffi_stop(self):
-        _dbg("Stoping", self)
+        _dbg("Stopping", self, self._watcher_stop)
         self._watcher_stop(self._watcher)
-        _dbg("Stoped", self)
+        _dbg("Stopped", self)
 
+    @_base.only_if_watcher
     def _watcher_ffi_ref(self):
         _dbg("Reffing", self)
         libuv.uv_ref(self._watcher)
 
+    @_base.only_if_watcher
     def _watcher_ffi_unref(self):
         _dbg("Unreffing", self)
         libuv.uv_unref(self._watcher)
@@ -196,14 +187,61 @@ class io(_base.IoMixin, watcher):
     _watcher_type = 'poll'
     _watcher_callback_name = '_gevent_poll_callback2'
 
-    EVENT_MASK = libuv.UV_READABLE | libuv.UV_WRITABLE | libuv.UV_DISCONNECT
+    # 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.
+
+    # Especially on Windows, we must also account for the case that a
+    # reference to this object has leaked (e.g., the socket object is
+    # still around), but the fileno has been closed and a new one
+    # opened. We must still get a new native watcher at that point. We
+    # handle this case by simply making sure that we don't even have
+    # a native watcher until the object is started, and we shut it down
+    # when the object is stopped.
+
+    # XXX: I was able to solve at least Windows test_ftplib.py issues with more of a
+    # careful use of io objects in socket.py, so delaying this entirely is at least
+    # temporarily on hold. Instead sticking with the _watcher_create
+    # function override for the moment.
+
+    #_watcher_init_on_init = False
 
-    _multiplex_watchers = None
+    EVENT_MASK = libuv.UV_READABLE | libuv.UV_WRITABLE | libuv.UV_DISCONNECT
 
     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
@@ -220,12 +258,57 @@ class io(_base.IoMixin, watcher):
     def _set_events(self, events):
         self._events = events
 
+    # This is what we'd do if we set _watcher_init_on_init to False:
+    # def start(self, *args, **kwargs):
+    #    assert self._watcher is None
+    #    self._watcher_full_init()
+    #    super(io, self).start(*args, **kwargs)
+
+    # Along with disposing of self._watcher in _watcher_ffi_stop.
+    # In that method, it's possible that we might be started again right after this,
+    # in which case we will create a new set of FFI objects.
+    # TODO: Does anything leak in that case? Verify...
+
     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.
+        _dbg("Stopping io watcher", self)
+        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 +326,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 +343,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,15 +369,16 @@ class io(_base.IoMixin, watcher):
     def _io_maybe_stop(self):
         for r in self._multiplex_watchers:
             w = r()
-            if w is None:
-                continue
-            if w.callback is not 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.
                 return
         # If we get here, nothing was started
         # so we can take ourself out of the polling set
         self.stop()
 
     def _io_start(self):
+        _dbg("IO start on behalf of multiplex", self)
         self.start(self._io_callback, pass_events=True)
 
     def multiplex(self, events):
@@ -435,7 +523,7 @@ class child(_SimulatedWithAsyncMixin,
         self._async.send()
 
 
-class async(_base.AsyncMixin, watcher):
+class async(_base.AsyncMixin, watcher): # XXX: Yeah, we know pylint:disable=assign-to-new-keyword
 
     def _watcher_ffi_init(self, args):
         # It's dangerous to have a raw, non-initted struct

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/known_failures.py

@@ -68,7 +68,8 @@ if sys.platform == 'win32':
             # thread in a timely fashion, leading to 'os.close(4) must
             # not succeed' in test_del_close. We have the same thing
             # with flushing and closing in test_newlines. Both of
-            # these are most commonly (only?) observed on Py27/64-bit
+            # these are most commonly (only?) observed on Py27/64-bit.
+            # They also appear on 64-bit 3.6 with libuv
             'FLAKY test__fileobject.py',
         ]
 

src/greentest/patched_tests_setup.py

@@ -14,9 +14,11 @@ import os
 import re
 
 TRAVIS = os.environ.get("TRAVIS") == "true"
+APPVEYOR = os.environ.get('APPVEYOR')
 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)
@@ -205,6 +207,19 @@ if LIBUV:
         'test_signal.SiginterruptTest.test_siginterrupt_off',
     ]
 
+    if PY3:
+
+        disabled_tests += [
+            # This test wants to pass an arbitrary fileno
+            # to a socket and do things with it. libuv doesn't like this,
+            # it raises EPERM. It is disabled on windows already.
+            # It depends on whether we had a fd already open and multiplexed with
+            'test_socket.GeneralModuleTests.test_unknown_socket_family_repr',
+            # And yes, there's a typo in some versions.
+            'test_socket.GeneralModuleTests.test_uknown_socket_family_repr',
+        ]
+
+
     if sys.platform.startswith('linux'):
         disabled_tests += [
             # crashes with EPERM, which aborts the epoll loop, even
@@ -224,6 +239,8 @@ if LIBUV:
             'test_selectors.PollSelectorTestCase.test_timeout',
         ]
 
+
+
     if WIN and PYPY:
         # From PyPy2-v5.9.0, using its version of tests,
         # which do work on darwin (and possibly linux?)
@@ -293,6 +310,24 @@ 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 += [
+            ]
+
+            if APPVEYOR:
+
+                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__core_stat.py

@@ -17,6 +17,8 @@ EV_USE_INOTIFY = getattr(gevent.core, 'EV_USE_INOTIFY', None)
 
 WIN = sys.platform.startswith('win')
 
+LIBUV = getattr(gevent.core, 'libuv', None)
+
 def test():
     try:
         open(filename, 'wb', buffering=0).close()
@@ -51,10 +53,10 @@ def test():
                 else:
                     raise
             else:
-                if WIN:
+                if WIN and not LIBUV:
                     # The ImportError is only raised for the first time;
                     # after that, the attribute starts returning None
-                    assert x is None, "Only None is supported on Windows"
+                    assert x is None, ("Only None is supported on Windows", x)
                 if none:
                     assert x is None, x
                 else:
@@ -67,6 +69,7 @@ def test():
         if now - start - DELAY <= 0.0:
             # Sigh. This is especially true on PyPy.
             assert WIN, ("Bad timer resolution expected on Windows, test is useless", start, now)
+            print("On windows, bad timer resolution prevents this test from running")
             return
         reaction = now - start - DELAY
         print('Watcher %s reacted after %.4f seconds (write)' % (watcher, reaction))

src/greentest/test__core_timer.py

@@ -20,7 +20,7 @@ def main():
     try:
         x.priority = 1
         raise AssertionError('must not be able to change priority of active watcher')
-    except AttributeError:
+    except (AttributeError, ValueError):
         pass
     loop.run()
     assert x.pending == 0, x.pending

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)

src/greentest/test__ssl.py

@@ -9,6 +9,9 @@ import test__socket
 import ssl
 
 
+import unittest
+from gevent.hub import LoopExit
+
 class TestSSL(test__socket.TestTCP):
 
     certfile = os.path.join(os.path.dirname(__file__), 'test_server.crt')
@@ -57,6 +60,16 @@ class TestSSL(test__socket.TestTCP):
                 client.close()
                 server_sock[0][0].close()
 
+    elif greentest.LIBUV:
+
+        def test_fullduplex(self):
+            try:
+                super(TestSSL, self).test_fullduplex()
+            except LoopExit:
+                # XXX: Unable to duplicate locally
+                raise unittest.SkipTest("libuv on Windows sometimes raises LoopExit")
+
+
     @greentest.ignores_leakcheck
     def test_empty_send(self):
         # Issue 719

src/greentest/testrunner.py

@@ -307,6 +307,9 @@ def main():
     if 'PYTHONFAULTHANDLER' not in os.environ:
         os.environ['PYTHONFAULTHANDLER'] = 'true'
 
+    if 'GEVENT_DEBUG' not in os.environ:
+        os.environ['GEVENT_DEBUG'] = 'error'
+
     tests = discover(options.tests, options.ignore, coverage)
     if options.discover:
         for cmd, options in tests: