Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed unhelpful error message in from_thread_run functions. #1513

Merged
merged 16 commits into from
May 21, 2020
Merged

Fixed unhelpful error message in from_thread_run functions. #1513

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
96cdeb2
Added helpful error message in the case the user passed incorrect typ…
May 12, 2020
015b220
Extracted "coroutine or TypeError" logic from Runner.spawn_impl, move…
May 13, 2020
126f7f4
Added sync check
May 13, 2020
417ba5b
Ran code formatter and added comments.
May 13, 2020
dc06cad
Added coroutine_or_error test.
May 13, 2020
dc1e5b8
Added TypeError tests to from_thread_run and from_thread_run_sync
May 13, 2020
5e4145d
Moved comprehensive test of callable from test_run.py to test_util.py
May 13, 2020
c74a829
Added error message check
May 14, 2020
ad27338
Marked some function stubs with #pragma no cover
May 14, 2020
57c1080
Fixed comment on spawn_impl. Moved ignore_coroutine_never_awaited_war…
May 15, 2020
f4c11e9
Fixed/added some comments. Removed unnecessary ignore_coroutine_never…
May 16, 2020
28b08d1
Fixed a comment and other formatting issues.
May 18, 2020
0f809d0
Remove another extra blank line
oremanj May 20, 2020
7a71a85
Remove a couple more of the extra added blank lines
oremanj May 20, 2020
7de4c84
Added newsfragment
May 20, 2020
f77c200
Update newsfragments/1244.bugfix.rst
May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions trio/_threads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import attr
import outcome

import inspect

import trio

from ._sync import CapacityLimiter
Expand Down Expand Up @@ -364,7 +366,7 @@ def from_thread_run(afn, *args, trio_token=None):
RuntimeError: if you try calling this from inside the Trio thread,
which would otherwise cause a deadlock.
AttributeError: if no ``trio_token`` was provided, and we can't infer
one from context.
one from context. Also if ``afn`` is not an async function.

**Locating a Trio Token**: There are two ways to specify which
`trio.run` loop to reenter:
Expand All @@ -377,6 +379,9 @@ def from_thread_run(afn, *args, trio_token=None):
"foreign" thread, spawned using some other framework, and still want
to enter Trio.
"""
if not inspect.iscoroutinefunction(afn):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, identifying async functions is rather subtler than just inspect.iscoroutinefunction, because async functions can be wrapped in a way that doesn't satisfy that test. (For example, Trio functions decorated with @enable_ki_protection are implemented using a wrapper that looks synchronous. functools.partial objects don't even satisfy inspect.isfunction, let alone inspect.iscoroutinefunction. All of these are perfectly valid to pass as an "async function" as long as the function being wrapped is async.)

The best way to see if something is an async function is to call it without await and use inspect.iscoroutine on the result. Of course, if it wasn't an async function, you just executed it, so it's best to do this only in the context where you were planning to call it anyway. In this case, that would be inside unprotected_afn below.

If you're not willing to call the function, you have to get a lot more special-cased to see if it's async or not. You can look inside the .func attribute of a functools.partial object, follow the .__wrapped__ attributes generally added by function wrappers, etc, but there's no approach that will definitely succeed for any async callable.

raise AttributeError("afn must be an asynchronous function")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user passed an argument of the wrong type, so you should raise TypeError.

In errors like this, it's also nice to mention in the message what specific thing the user sent you. For example, you could use f"{afn!r} must be an asynchronous function" to insert repr(afn) into the exception message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I knew the error message needed some work.


def callback(q, afn, args):
@disable_ki_protection
async def unprotected_afn():
Expand Down Expand Up @@ -409,7 +414,7 @@ def from_thread_run_sync(fn, *args, trio_token=None):
RuntimeError: if you try calling this from inside the Trio thread,
which would otherwise cause a deadlock.
AttributeError: if no ``trio_token`` was provided, and we can't infer
one from context.
one from context. Also if ``fn`` is not a sync function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leftover from your previous use of AttributeError for this condition, but you're now using TypeError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You marked this as resolved but you didn't update the docstring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have not updated the docstring despite having marked this as resolved again. The request is to remove the text "Also if fn is not a sync function" from the description for AttributeError, because you no longer raise AttributeError in that case.


**Locating a Trio Token**: There are two ways to specify which
`trio.run` loop to reenter:
Expand All @@ -422,6 +427,9 @@ def from_thread_run_sync(fn, *args, trio_token=None):
"foreign" thread, spawned using some other framework, and still want
to enter Trio.
"""
if not (inspect.isfunction(fn) and not inspect.iscoroutinefunction(fn)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed above, these won't tell you what you want. Better to wait and raise an error if the result of calling the function satisfies inspect.iscoroutine. (If it does, you'll also want to close() it before returning, so you don't get an extra "coroutine was never awaited" warning.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw your comment on #1504, should I look into Runner.spawn_impl() for the "async fn or error" logic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a useful extension if you feel like it! The relevant code is toward the top of spawn_impl(), with the comments about "Give good error for [...]"

raise AttributeError("fn must be a synchronous function")

def callback(q, fn, args):
@disable_ki_protection
def unprotected_fn():
Expand Down