Merge pull request #1379 from gevent/issue1363

Performance and correctness improvements for Greenlets.

CHANGES.rst

@@ -51,6 +51,19 @@
 - Win: Make ``examples/process.py`` do something useful. See
   :pr:`1378` by Robert Iannucci.
 
+- Fix certain operations on a Greenlet in an invalid state (with an
+  invalid parent) to raise a `TypeError` sooner rather than an
+  `AttributeError` later. This is also slightly faster on CPython with
+  Cython. Inspired by :issue:`1363` as reported by Carson Ip. This
+  means that some extreme corner cases that might have passed by
+  replacing a Greenlet's parent with something that's not a gevent hub
+  now no longer will.
+
+- Fix: The ``spawning_stack`` for Greenlets on CPython should now have
+  correct line numbers in more cases.
+
+- Spawning greenlets can be up to 10% faster.
+
 1.4.0 (2019-01-04)
 ==================
 

src/gevent/__hub_primitives.pxd

@@ -39,7 +39,7 @@ cdef inline void greenlet_init():
 
 
 cdef class WaitOperationsGreenlet(SwitchOutGreenletWithLoop):
-
+    # The Hub will extend this class.
     cpdef wait(self, watcher)
     cpdef cancel_wait(self, watcher, error, close_watcher=*)
     cpdef _cancel_wait(self, watcher, error, close_watcher)

src/gevent/__init__.py

@@ -21,7 +21,7 @@ _version_info = namedtuple('version_info',
 #: .. deprecated:: 1.2
 #:  Use ``pkg_resources.parse_version(__version__)`` (or the equivalent
 #:  ``packaging.version.Version(__version__)``).
-version_info = _version_info(1, 4, 0, 'dev', 0)
+version_info = _version_info(1, 5, 0, 'dev', 0)
 
 #: The human-readable PEP 440 version identifier.
 #: Use ``pkg_resources.parse_version(__version__)`` or

src/gevent/_greenlet.pxd

@@ -4,6 +4,7 @@ cimport cython
 from gevent.__ident cimport IdentRegistry
 from gevent.__hub_local cimport get_hub_noargs as get_hub
 from gevent.__waiter cimport Waiter
+from gevent.__greenlet_primitives cimport SwitchOutGreenletWithLoop
 
 cdef bint _PYPY
 cdef sys_getframe
@@ -15,7 +16,12 @@ cdef InvalidSwitchError
 cdef extern from "greenlet/greenlet.h":
 
     ctypedef class greenlet.greenlet [object PyGreenlet]:
-        pass
+        # Defining this as a void* means we can't access it as a python attribute
+        # in the Python code; but we can't define it as a greenlet because that doesn't
+        # properly handle the case that it can be NULL. So instead we inline a getparent
+        # function that does the same thing as the green_getparent accessor but without
+        # going through the overhead of generic attribute lookup.
+        cdef void* parent
 
     # These are actually macros and so much be included
     # (defined) in each .pxd, as are the two functions
@@ -27,6 +33,17 @@ cdef extern from "greenlet/greenlet.h":
 cdef inline greenlet getcurrent():
     return PyGreenlet_GetCurrent()
 
+cdef inline object get_generic_parent(greenlet s):
+    # We don't use any typed functions on the return of this,
+    # so save the type check by making it just an object.
+    if s.parent != NULL:
+        return <object>s.parent
+
+cdef inline SwitchOutGreenletWithLoop get_my_hub(greenlet s):
+    # Must not be called with s = None
+    if s.parent != NULL:
+        return <object>s.parent
+
 cdef bint _greenlet_imported
 
 cdef inline void greenlet_init():
@@ -44,12 +61,25 @@ cdef extern from "frameobject.h":
 
     ctypedef class types.FrameType [object PyFrameObject]:
         cdef CodeType f_code
-        cdef int f_lineno
-        # We can't declare this in the object, because it's
+        # Accessing the f_lineno directly doesn't work. There is an accessor
+        # function, PyFrame_GetLineNumber that is needed to turn the raw line number
+        # into the executing line number.
+        # cdef int f_lineno
+        # We can't declare this in the object as an object, because it's
         # allowed to be NULL, and Cython can't handle that.
         # We have to go through the python machinery to get a
-        # proper None instead.
-        # cdef FrameType f_back
+        # proper None instead, or use an inline function.
+        cdef void* f_back
+
+    int PyFrame_GetLineNumber(FrameType frame)
+
+@cython.nonecheck(False)
+cdef inline FrameType get_f_back(FrameType frame):
+    if frame.f_back != NULL:
+        return <FrameType>frame.f_back
+
+cdef inline int get_f_lineno(FrameType frame):
+    return PyFrame_GetLineNumber(frame)
 
 cdef void _init()
 
@@ -75,25 +105,20 @@ cdef class _Frame:
 
 
 @cython.final
-@cython.locals(frames=list,frame=FrameType)
-cdef inline list _extract_stack(int limit)
-
-@cython.final
-@cython.locals(previous=_Frame, frame=tuple, f=_Frame)
-cdef _Frame _Frame_from_list(list frames)
-
+@cython.locals(frame=FrameType,
+               newest_Frame=_Frame,
+               newer_Frame=_Frame,
+               older_Frame=_Frame)
+cdef inline _Frame _extract_stack(int limit)
 
 cdef class Greenlet(greenlet):
     cdef readonly object value
     cdef readonly tuple args
     cdef readonly dict kwargs
     cdef readonly object spawning_greenlet
+    cdef readonly _Frame spawning_stack
     cdef public dict spawn_tree_locals
 
-    # This is accessed with getattr() dynamically so it
-    # must be visible to Python
-    cdef readonly list _spawning_stack_frames
-
     cdef list _links
     cdef tuple _exc_info
     cdef object _notifier
@@ -108,10 +133,11 @@ cdef class Greenlet(greenlet):
     cpdef rawlink(self, object callback)
     cpdef str _formatinfo(self)
 
+    # This is a helper function for a @property getter;
+    # defining locals() for a @property doesn't seem to work.
     @cython.locals(reg=IdentRegistry)
     cdef _get_minimal_ident(self)
 
-
     cdef bint __started_but_aborted(self)
     cdef bint __start_cancelled_by_kill(self)
     cdef bint __start_pending(self)
@@ -132,8 +158,6 @@ cdef class Greenlet(greenlet):
     # TypeError: wrap() takes exactly one argument (0 given)
     # cpdef _raise_exception(self)
 
-
-
 # Declare a bunch of imports as cdefs so they can
 # be accessed directly as static vars without
 # doing a module global lookup. This is especially important
@@ -165,6 +189,7 @@ cdef class _dummy_event:
 cdef _dummy_event _cancelled_start_event
 cdef _dummy_event _start_completed_event
 
+cpdef _kill(Greenlet glet, object exception, object waiter)
 
 @cython.locals(diehards=list)
 cdef _killall3(list greenlets, object exception, object waiter)

src/gevent/_hub_primitives.py

@@ -54,9 +54,15 @@ class WaitOperationsGreenlet(SwitchOutGreenletWithLoop): # pylint:disable=undefi
         try:
             result = waiter.get()
             if result is not waiter:
-                raise InvalidSwitchError('Invalid switch into %s: %r (expected %r)' % (
-                    getcurrent(), # pylint:disable=undefined-variable
-                    result, waiter))
+                raise InvalidSwitchError(
+                    'Invalid switch into %s: got %r (expected %r; waiting on %r with %r)' % (
+                        getcurrent(), # pylint:disable=undefined-variable
+                        result,
+                        waiter,
+                        self,
+                        watcher
+                    )
+                )
         finally:
             watcher.stop()
 

src/gevent/_ident.py

@@ -27,7 +27,7 @@ class ValuedWeakRef(ref):
 
 class IdentRegistry(object):
     """
-    Maintains a unique mapping of (small) positive integer identifiers
+    Maintains a unique mapping of (small) non-negative integer identifiers
     to objects that can be weakly referenced.
 
     It is guaranteed that no two objects will have the the same

src/gevent/tests/test__greenlet.py

@@ -531,22 +531,22 @@ class TestBasic(greentest.TestCase):
 
         g = gevent.Greenlet(func, 0.01, return_value=5)
         g.rawlink(link_test.append) # use rawlink to avoid timing issues on Appveyor/Travis (not always successful)
-        assert not g, bool(g)
-        assert not g.dead
-        assert not g.started
-        assert not g.ready()
-        assert not g.successful()
-        assert g.value is None
-        assert g.exception is None
+        self.assertFalse(g, g)
+        self.assertFalse(g.dead, g)
+        self.assertFalse(g.started, g)
+        self.assertFalse(g.ready(), g)
+        self.assertFalse(g.successful(), g)
+        self.assertIsNone(g.value, g)
+        self.assertIsNone(g.exception, g)
 
         g.start()
-        assert g  # changed
-        assert not g.dead
-        assert g.started  # changed
-        assert not g.ready()
-        assert not g.successful()
-        assert g.value is None
-        assert g.exception is None
+        self.assertTrue(g, g) # changed
+        self.assertFalse(g.dead, g)
+        self.assertTrue(g.started, g) # changed
+        self.assertFalse(g.ready(), g)
+        self.assertFalse(g.successful(), g)
+        self.assertIsNone(g.value, g)
+        self.assertIsNone(g.exception, g)
 
         gevent.sleep(0.001)
         self.assertTrue(g)
@@ -559,14 +559,15 @@ class TestBasic(greentest.TestCase):
         self.assertFalse(link_test)
 
         gevent.sleep(0.02)
-        assert not g
-        assert g.dead
-        assert not g.started
-        assert g.ready()
-        assert g.successful()
-        assert g.value == 5
-        assert g.exception is None  # not changed
-        assert link_test == [g] or greentest.RUNNING_ON_CI, link_test  # changed
+        self.assertFalse(g, g) # changed
+        self.assertTrue(g.dead, g) # changed
+        self.assertFalse(g.started, g) # changed
+        self.assertTrue(g.ready(), g) # changed
+        self.assertTrue(g.successful(), g) # changed
+        self.assertEqual(g.value, 5) # changed
+        self.assertIsNone(g.exception, g)
+
+        self.assertTrue(link_test == [g] or greentest.RUNNING_ON_CI, link_test)  # changed
 
     def test_error_exit(self):
         link_test = []
@@ -755,28 +756,24 @@ class TestBasic(greentest.TestCase):
         finally:
             greenlet.sys_getframe = ogf
 
-    def test_spawn_length_nested_doesnt_grow(self):
-        # https://github.com/gevent/gevent/pull/1374
-        def doit1():
-            getcurrent()._spawning_stack_frames.append('not copied')
-            return gevent.spawn(doit2)
-
-        def doit2():
-            return gevent.spawn(doit3)
+    def test_minimal_ident_parent_not_hub(self):
 
-        def doit3():
-            current = gevent.getcurrent()
-
-            return current
-
-
-        g1 = gevent.spawn(doit1)
-        g1.join()
-        g2 = g1.value
-        g2.join()
+        g = gevent.spawn(lambda: 1)
+        self.assertIs(g.parent, gevent.get_hub())
+        g.parent = getcurrent()
+        try:
+            self.assertIsNot(g.parent, gevent.get_hub())
 
-        g3 = g2.value
-        self.assertNotIn('not copied', g3._spawning_stack_frames)
+            with self.assertRaisesRegex((TypeError, # Cython
+                                         AttributeError), # PyPy
+                                        'Cannot convert|ident_registry'):
+                getattr(g, 'minimal_ident')
+        finally:
+            # Attempting to switch into this later, when we next cycle the
+            # loop, would raise an InvalidSwitchError if we don't put
+            # things back the way they were (or kill the greenlet)
+            g.parent = gevent.get_hub()
+            g.kill()
 
 
 

src/gevent/util.py

@@ -417,9 +417,11 @@ class GreenletTree(object):
     def __render_children(self, tree):
         children = sorted(self.child_trees,
                           key=lambda c: (
-                              # raw greenlets first
+                              # raw greenlets first. Note that we could be accessing
+                              # minimal_ident for a hub from a different thread, which isn't
+                              # technically thread safe.
                               getattr(c, 'minimal_ident', -1),
-                              # running greenlets first
+                              # running greenlets next
                               getattr(c, 'ready', _ready)(),
                               id(c.parent)))
         for n, child in enumerate(children):