Merge pull request #191 from njsmith/better-errors-on-mixups

Give better error messages if users mix us up with asyncio

trio/_core/_run.py

@@ -25,6 +25,7 @@
 from ._result import Result, Error, Value
 from ._traps import (
     yield_briefly_no_cancel, Abort, yield_indefinitely,
+    YieldBrieflyNoCancel, YieldIndefinitely,
 )
 from ._ki import (
     LOCALS_KEY_KI_PROTECTION_ENABLED, currently_ki_protected, ki_manager,
@@ -635,16 +636,96 @@ def reschedule(self, task, next_send=Value(None)):
     def spawn_impl(
             self, async_fn, args, nursery, name,
             *, ki_protection_enabled=False):
+
+        ######
+        # Make sure the nursery is in working order
+        ######
+
         # This sorta feels like it should be a method on nursery, except it
         # has to handle nursery=None for init. And it touches the internals of
         # all kinds of objects.
         if nursery is not None and nursery._closed:
             raise RuntimeError("Nursery is closed to new arrivals")
         if nursery is None:
             assert self.init_task is None
-        coro = async_fn(*args)
+
+        ######
+        # Call the function and get the coroutine object, while giving helpful
+        # errors for common mistakes.
+        ######
+
+        def _return_value_looks_like_wrong_library(value):
+            # Returned by legacy @asyncio.coroutine functions, which includes
+            # a surprising proportion of asyncio builtins.
+            if inspect.isgenerator(value):
+                return True
+            # The protocol for detecting an asyncio Future-like object
+            if getattr(value, "_asyncio_future_blocking", None) is not None:
+                return True
+            # asyncio.Future doesn't have _asyncio_future_blocking until
+            # 3.5.3. We don't want to import asyncio, but this janky check
+            # should work well enough for our purposes. And it also catches
+            # tornado Futures and twisted Deferreds. By the time we're calling
+            # this function, we already know something has gone wrong, so a
+            # heuristic is pretty safe.
+            if value.__class__.__name__ in ("Future", "Deferred"):
+                return True
+            return False
+
+        try:
+            coro = async_fn(*args)
+        except TypeError:
+            # Give good error for: nursery.spawn(trio.sleep(1))
+            if inspect.iscoroutine(async_fn):
+                raise TypeError(
+                    "trio was expecting an async function, but instead it got "
+                    "a coroutine object {async_fn!r}\n"
+                    "\n"
+                    "Probably you did something like:\n"
+                    "\n"
+                    "  trio.run({async_fn.__name__}(...))       # incorrect!\n"
+                    "  nursery.spawn({async_fn.__name__}(...))  # incorrect!\n"
+                    "\n"
+                    "Instead, you want (notice the parentheses!):\n"
+                    "\n"
+                    "  trio.run({async_fn.__name__}, ...)       # correct!\n"
+                    "  nursery.spawn({async_fn.__name__}, ...)  # correct!"
+                    .format(async_fn=async_fn)) from None
+
+            # Give good error for: nursery.spawn(asyncio.sleep(1))
+            if _return_value_looks_like_wrong_library(async_fn):
+                raise TypeError(
+                    "trio was expecting an async function, but instead it got "
+                    "{!r} – are you trying to use a library written for "
+                    "asyncio/twisted/tornado or similar? That won't work "
+                    "without some sort of compatibility shim."
+                    .format(async_fn)) from None
+
+            raise
+
+        # We can't check iscoroutinefunction(async_fn), because that will fail
+        # for things like functools.partial objects wrapping an async
+        # function. So we have to just call it and then check whether the
+        # result is a coroutine object.
         if not inspect.iscoroutine(coro):
-            raise TypeError("spawn expected an async function")
+            # Give good error for: nursery.spawn(asyncio.sleep, 1)
+            print(coro)
+            if _return_value_looks_like_wrong_library(coro):
+                raise TypeError(
+                    "spawn got unexpected {!r} – are you trying to use a "
+                    "library written for asyncio/twisted/tornado or similar? "
+                    "That won't work without some sort of compatibility shim."
+                    .format(coro))
+            # Give good error for: nursery.spawn(some_sync_fn)
+            raise TypeError(
+                "trio expected an async function, but {!r} appears to be "
+                "synchronous"
+                .format(getattr(async_fn, "__qualname__", async_fn)))
+
+        ######
+        # Set up the Task object
+        ######
+
         if name is None:
             name = async_fn
         if isinstance(name, functools.partial):
@@ -1132,18 +1213,6 @@ def run(async_fn, *args, clock=None, instruments=[],
     if hasattr(GLOBAL_RUN_CONTEXT, "runner"):
         raise RuntimeError("Attempted to call run() from inside a run()")
 
-    if inspect.iscoroutine(async_fn):
-        raise TypeError(
-            "trio.run received unexpected coroutine object {}.\n"
-            "Probably you did something like this:\n"
-            "\n"
-            "    trio.run(my_function(1))  # incorrect!\n"
-            "\n"
-            "Instead, do this:\n"
-            "\n"
-            "    trio.run(my_function, 1)  # correct!"
-            .format(async_fn))
-
     if clock is None:
         clock = SystemClock()
     instruments = list(instruments)
@@ -1283,18 +1352,26 @@ def run_impl(runner, async_fn, args):
                 runner.task_exited(task, final_result)
             else:
                 task._schedule_points += 1
-                yield_fn, *args = msg
-                if yield_fn is yield_briefly_no_cancel:
+                if msg is YieldBrieflyNoCancel:
                     runner.reschedule(task)
-                else:
-                    assert yield_fn is yield_indefinitely
+                elif type(msg) is YieldIndefinitely:
                     task._cancel_points += 1
-                    task._abort_func, = args
+                    task._abort_func = msg.abort_func
                     # KI is "outside" all cancel scopes, so check for it
                     # before checking for regular cancellation:
                     if runner.ki_pending and task is runner.main_task:
                         task._attempt_delivery_of_pending_ki()
                     task._attempt_delivery_of_any_pending_cancel()
+                else:
+                    exc = TypeError(
+                        "trio.run received unrecognized yield message {!r}. "
+                        "Are you trying to use a library written for some "
+                        "other framework like asyncio? That won't work "
+                        "without some kind of compatibility shim."
+                        .format(msg))
+                    # There's really no way to resume this task, so abandon it
+                    # and propagate the exception into the task's spawner.
+                    runner.task_exited(task, Error(exc))
 
             runner.instrument("after_task_step", task)
             del GLOBAL_RUN_CONTEXT.task

trio/_core/_traps.py

@@ -4,6 +4,8 @@
 import enum
 from functools import wraps
 
+import attr
+
 from . import _hazmat
 
 __all__ = ["yield_briefly_no_cancel", "Abort", "yield_indefinitely"]
@@ -19,13 +21,21 @@ async def wrapper(*args, **kwargs):
         return await fn(*args, **kwargs)
     return wrapper
 
+
+# This class object is used as a singleton.
+# Not exported in the trio._core namespace, but imported directly by _run.
+class YieldBrieflyNoCancel:
+    pass
+
+
 @_hazmat
 @asyncfunction
 def yield_briefly_no_cancel():
     """Introduce a schedule point, but not a cancel point.
 
     """
-    return (yield (yield_briefly_no_cancel,)).unwrap()
+    return (yield YieldBrieflyNoCancel).unwrap()
+
 
 # Return values for abort functions
 @_hazmat
@@ -41,9 +51,16 @@ class Abort(enum.Enum):
     SUCCEEDED = 1
     FAILED = 2
 
+
+# Not exported in the trio._core namespace, but imported directly by _run.
+@attr.s(frozen=True)
+class YieldIndefinitely:
+    abort_func = attr.ib()
+
+
 @_hazmat
 @asyncfunction
-def yield_indefinitely(abort_fn):
+def yield_indefinitely(abort_func):
     """Put the current task to sleep, with cancellation support.
 
     This is the lowest-level API for blocking in trio. Every time a
@@ -57,7 +74,7 @@ def yield_indefinitely(abort_fn):
     arrangements for "someone" to call :func:`reschedule` on the current task
     at some later point.
 
-    Then you call :func:`yield_indefinitely`, passing in ``abort_fn``, an
+    Then you call :func:`yield_indefinitely`, passing in ``abort_func``, an
     "abort callback".
 
     (Terminology: in trio, "aborting" is the process of attempting to
@@ -70,10 +87,10 @@ def yield_indefinitely(abort_fn):
        was passed to :func:`reschedule`.
 
     2. The call's context transitions to a cancelled state (e.g. due to a
-       timeout expiring). When this happens, the ``abort_fn`` is called. It's
+       timeout expiring). When this happens, the ``abort_func`` is called. It's
        interface looks like::
 
-           def abort_fn(raise_cancel):
+           def abort_func(raise_cancel):
                ...
                return trio.hazmat.Abort.SUCCEEDED  # or FAILED
 
@@ -91,14 +108,14 @@ def abort_fn(raise_cancel):
        the cancellation altogether: wait for the operation to complete and
        then reschedule and continue as normal. (For example, this is what
        :func:`trio.run_in_worker_thread` does if cancellation is disabled.)
-       The other possibility is that the ``abort_fn`` does succeed in
+       The other possibility is that the ``abort_func`` does succeed in
        cancelling the operation, but for some reason isn't able to report that
        right away. (Example: on Windows, it's possible to request that an
        async ("overlapped") I/O operation be cancelled, but this request is
        *also* asynchronous – you don't find out until later whether the
        operation was actually cancelled or not.)  To report a delayed
        cancellation, then you should reschedule the task yourself, and call
-       the ``raise_cancel`` callback passed to ``abort_fn`` to raise a
+       the ``raise_cancel`` callback passed to ``abort_func`` to raise a
        :exc:`~trio.Cancelled` (or possibly :exc:`KeyboardInterrupt`) exception
        into this task. Either of the approaches sketched below can work::
 
@@ -122,17 +139,17 @@ def abort(inner_raise_cancel):
               # raises the error
               outer_raise_cancel()
 
-       In any case it's guaranteed that we only call the ``abort_fn`` at most
+       In any case it's guaranteed that we only call the ``abort_func`` at most
        once per call to :func:`yield_indefinitely`.
 
     .. warning::
 
-       If your ``abort_fn`` raises an error, or returns any value other than
+       If your ``abort_func`` raises an error, or returns any value other than
        :data:`Abort.SUCCEEDED` or :data:`Abort.FAILED`, then trio will crash
        violently. Be careful! Similarly, it is entirely possible to deadlock a
        trio program by failing to reschedule a blocked task, or cause havoc by
        calling :func:`reschedule` too many times. Remember what we said up
        above about how you should use a higher-level API if at all possible?
 
     """
-    return (yield (yield_indefinitely, abort_fn)).unwrap()
+    return (yield YieldIndefinitely(abort_func)).unwrap()