Merge pull request #1720 from gevent/issue1663

Fix #1663 by more gracefully handling non-type arguments.

docs/changes/1663.bugfix

+Incorrectly passing an exception *instance* instead of an exception
+*type* to `gevent.Greenlet.kill` or `gevent.killall` no longer prints
+an exception to stderr.

src/gevent/_abstract_linkable.py

@@ -223,6 +223,12 @@ class AbstractLinkable(object):
         # links in order. Lets the ``links`` list be mutated,
         # and only notifies up to the last item in the list, in case
         # objects are added to it.
+        if not links:
+            # HMM. How did we get here? Running two threads at once?
+            # Seen once on Py27/Win/Appveyor
+            # https://ci.appveyor.com/project/jamadden/gevent/builds/36875645/job/9wahj9ft4h4qa170
+            return []
+
         only_while_ready = not self._notify_all
         final_link = links[-1]
         done = set() # of ids

src/gevent/greenlet.py

@@ -449,7 +449,14 @@ class Greenlet(greenlet):
         self._start_event.close()
 
     def __handle_death_before_start(self, args):
-        # args is (t, v, tb) or simply t or v
+        # args is (t, v, tb) or simply t or v.
+        # The last two cases are transformed into (t, v, None);
+        # if the single argument is an exception type, a new instance
+        # is created; if the single argument is not an exception type and also
+        # not an exception, it is wrapped in a BaseException (this is not
+        # documented, but should result in better behaviour in the event of a
+        # user error---instead of silently printing something to stderr, we still
+        # kill the greenlet).
         if self._exc_info is None and self.dead:
             # the greenlet was never switched to before and it will
             # never be; _report_error was not called, the result was
@@ -462,12 +469,15 @@ class Greenlet(greenlet):
             # the greenlet).
             if len(args) == 1:
                 arg = args[0]
-                if issubclass(arg, BaseException):
+                if isinstance(arg, type) and issubclass(arg, BaseException):
                     args = (arg, arg(), None)
                 else:
                     args = (type(arg), arg, None)
             elif not args:
                 args = (GreenletExit, GreenletExit(), None)
+            if not issubclass(args[0], BaseException):
+                # Random non-type, non-exception arguments.
+                args = (BaseException, BaseException(args), None)
             assert issubclass(args[0], BaseException)
             self.__report_error(args)
 
@@ -734,7 +744,7 @@ class Greenlet(greenlet):
             a :meth:`link` or :meth:`rawlink` (cheaper) may be a safer way to
             clean up resources.
 
-        See also :func:`gevent.kill`.
+        See also :func:`gevent.kill` and :func:`gevent.killall`.
 
         :keyword type exception: The type of exception to raise in the greenlet. The default
             is :class:`GreenletExit`, which indicates a :meth:`successful` completion
@@ -1096,7 +1106,7 @@ def killall(greenlets, exception=GreenletExit, block=True, timeout=None):
         calling this method will never be switched to. This makes this function
         behave like :meth:`Greenlet.kill`. This does not apply to raw greenlets.
     .. versionchanged:: 1.5a3
-       Now accepts raw greenlets created by :func:`gevent.spawn_raw`.
+        Now accepts raw greenlets created by :func:`gevent.spawn_raw`.
     """
 
     need_killed = [] # type: list

src/gevent/tests/test__greenlet.py

@@ -732,33 +732,43 @@ class TestBasic(greentest.TestCase):
 
 class TestKill(greentest.TestCase):
 
-    def __assertKilled(self, g):
+    def __assertKilled(self, g, successful):
         self.assertFalse(g)
         self.assertTrue(g.dead)
         self.assertFalse(g.started)
         self.assertTrue(g.ready())
-        self.assertTrue(g.successful(), (repr(g), g.value, g.exception))
-        self.assertIsInstance(g.value, gevent.GreenletExit)
-        self.assertIsNone(g.exception)
+        if successful:
+            self.assertTrue(g.successful(), (repr(g), g.value, g.exception))
+            self.assertIsInstance(g.value, gevent.GreenletExit)
+            self.assertIsNone(g.exception)
+        else:
+            self.assertFalse(g.successful(), (repr(g), g.value, g.exception))
+            self.assertNotIsInstance(g.value, gevent.GreenletExit)
+            self.assertIsNotNone(g.exception)
 
-    def assertKilled(self, g):
-        self.__assertKilled(g)
+    def assertKilled(self, g, successful=True):
+        self.__assertKilled(g, successful)
         gevent.sleep(0.01) # spin the loop to make sure it doesn't run.
-        self.__assertKilled(g)
+        self.__assertKilled(g, successful)
 
-    def __kill_greenlet(self, g, block, killall):
+    def __kill_greenlet(self, g, block, killall, exc=None):
+        if exc is None:
+            exc = gevent.GreenletExit
         if killall:
-            killer = functools.partial(gevent.killall, [g], block=block)
+            killer = functools.partial(gevent.killall, [g],
+                                       exception=exc, block=block)
         else:
-            killer = functools.partial(g.kill, block=block)
+            killer = functools.partial(g.kill, exception=exc, block=block)
         killer()
         if not block:
             # Must spin the loop to take effect (if it was scheduled)
             gevent.sleep(timing.SMALLEST_RELIABLE_DELAY)
-        self.assertKilled(g)
+
+        successful = exc is None or (isinstance(exc, type) and issubclass(exc, gevent.GreenletExit))
+        self.assertKilled(g, successful)
         # kill second time must not hurt
         killer()
-        self.assertKilled(g)
+        self.assertKilled(g, successful)
 
     @staticmethod
     def _run_in_greenlet(result_collector):
@@ -772,7 +782,7 @@ class TestKill(greentest.TestCase):
     _after_kill_greenlet = _start_greenlet
 
 
-    def _do_test(self, block, killall):
+    def _do_test(self, block, killall, exc=None):
         link_test = []
         result = []
         g = gevent.Greenlet(self._run_in_greenlet, result)
@@ -780,7 +790,7 @@ class TestKill(greentest.TestCase):
 
         self._start_greenlet(g)
 
-        self.__kill_greenlet(g, block, killall)
+        self.__kill_greenlet(g, block, killall, exc)
 
         self._after_kill_greenlet(g)
 
@@ -793,12 +803,36 @@ class TestKill(greentest.TestCase):
     def test_non_block(self):
         self._do_test(block=False, killall=False)
 
-    def test_block_killal(self):
+    def test_block_killall(self):
         self._do_test(block=True, killall=True)
 
     def test_non_block_killal(self):
         self._do_test(block=False, killall=True)
 
+    def test_non_type_exception(self):
+        self._do_test(block=True, killall=False, exc=Exception())
+
+    def test_non_type_exception_non_block(self):
+        self._do_test(block=False, killall=False, exc=Exception())
+
+    def test_non_type_exception_killall(self):
+        self._do_test(block=True, killall=True, exc=Exception())
+
+    def test_non_type_exception_killall_non_block(self):
+        self._do_test(block=False, killall=True, exc=Exception())
+
+    def test_non_exc_exception(self):
+        self._do_test(block=True, killall=False, exc=42)
+
+    def test_non_exc_exception_non_block(self):
+        self._do_test(block=False, killall=False, exc=42)
+
+    def test_non_exc_exception_killall(self):
+        self._do_test(block=True, killall=True, exc=42)
+
+    def test_non_exc_exception_killall_non_block(self):
+        self._do_test(block=False, killall=True, exc=42)
+
 
 class TestKillAfterStart(TestKill):