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.

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

Use asyncio.iscoroutine instead of inspect.isawaitable #74

Closed
wants to merge 1 commit into from
Closed

Use asyncio.iscoroutine instead of inspect.isawaitable #74

wants to merge 1 commit into from

Conversation

corydolphin
Copy link

@corydolphin corydolphin commented Jan 5, 2020

R.e. the performance issues seen here: #54

Long term, it seems the best fix would be to minimize the usage of iscoroutine, in favor of inspecting and caching whether or not the function iscoroutinefunction.

As a shorter term fix, this change switches form inspect.isawaitable to asyncio.iscoroutine. inspect.isawaitable is pure Python, while asyncio.iscoroutine is optimized in C and has a smarter type-cache. Reading through their implementations and comments, it isn't clear to me whether or not there is a semantic difference between the two, I'm relying on integration tests here to catch that. If there is a difference, I can update to use an early-exiting or to at least take the fast path when possible, assuming standard coroutines are the common case 🤞

@corydolphin corydolphin requested a review from Cito as a code owner January 5, 2020 22:18
@corydolphin
Copy link
Author

Darn. The one issue is that iscoroutine returns True if the value is a generator, this breaks the list resolver in the case of a generator.

@Cito
Copy link
Member

Cito commented Jan 6, 2020

Thanks for your PR. I'm currently very busy, but will look into this when I find time. Maybe we can find a solution for that issue with generators. We also need to check how much this change is visible in the performance tests (activate with --benchmark-enable when running the tests).

@Cito Cito self-assigned this Jan 6, 2020
@Cito Cito added the investigate Needs investigaton or experimentation label Jan 6, 2020
@syrusakbary
Copy link
Member

Coroutines and awaitables are different things.

Coroutines refer to async functions, while awaitables refer to the result of running async functions.

isawaitable is the right thing to ask for, if you are asking in the data. We could ask on iscoroutine in the function, but this make the following to not work:

async def my_async_func():
  return 1

def resolve_x(root, info, **args):
  return my_async_func()

assert iscoroutine(resolve_x) == False
assert isawaitable(resove_x()) == True

Closing the PR

@Cito
Copy link
Member

Cito commented Mar 21, 2020

@syrusakbary asyncio.iscoroutine actually checks for coroutine objects. In your example, you do not call resolve_x() in the first assert statement, otherwise it would be equal to True.

I have removed my last misleading comment. The problem with asyncio.iscoroutine is not that it does not detect old-syle generator-based coroutine.

But the problems with asyncio.iscoroutine instead of isawaitable are:

  1. it returns True for generators (as @corydolphin already commented),
  2. it returns False for Futures because they are not coroutines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Needs investigaton or experimentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants