FastAPI and Strawberry, Losing Concurrency when Following Intro Example

[Chris927]

In short, using the example on how to integrate Strawberry with FastAPI, you end up with a service that handles GraphQL requests sequentially. This defeats the purpose of FastAPI.

My request: Make this clear in the docs. Possibly by changing the example to async query functions, and adding a warning about using sync query functions.

I've documented my understanding and findings in this repo.

Am I missing anything? Let me know! I am happy to submit a CR to update the docs, if adequate.

Upvote & Fund

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[patrick91]

Thanks for the repo! I think that's quite useful, I'll double check that soon 😊

My understand is that simple resolver (ie resolver not doing anything with network) can be sync as it shouldn't impact the concurrency, but maybe I'm wrong?

[ThirVondukr]

Can confirm that, I think it should be expected from async frameworks that doing any synchronous IO would block the event loop if it's not mentioned otherwise in the documentation 🤔
It wouldn't make sense for strawberry to run all sync resolvers in a threadpool since that would be quite inefficient

[Chris927]

@ThirVondukr thanks for your comment!

From the docs of FastAPI:

You can mix def and async def in your path operation functions as much as you need and define each one using the best option for you. FastAPI will do the right thing with them.

The "right thing" in case of a sync path operation function for FastAPI is to run that path operation function in a thread pool.

I disagree with your statement that running sync resolvers in a threadpool would be inefficient: Enabling concurrency through a threadpool is one way, enabling concurrency using async IO and an event loop is another way. Both ways have their own performance characteristics. Simply not getting any concurrency (as in the case of using sync resolvers naively in Strawberry) is certainly worse than using a threadpool, in most scenarios.

To illustrate, I've expanded my testing app, and done a performance test with concurrency on a sync FastAPI route (not involving Strawberry).

I suggest to make one of two possible improvements:

1. In the docs on how to integrate with FastAPI, tell your users that they do not get the same "threadpool magic" they get with FastAPI. To help them even further, you could explain how to implement a wrapper to get the benefits of a threadpool when using sync resolvers.
2. Implement a similar approach to FastAPI.

Keen to hear your thoughts!

[ThirVondukr]

The issue with making sync resolvers magically async is that graphql potentially would have to resolve thousands of them per single request 🤔, that's why I think it's inefficient

By the way, currently starwberry runs resolvers using await_maybe:
https://github.com/strawberry-graphql/strawberry/blob/ff583b68ba00f6ac6360bbc1639206a742543326/strawberry/utils/await_maybe.py

async def await_maybe(value: AwaitableOrValue[T]) -> T:
    if inspect.isawaitable(value):
        return await value

    return cast(T, value)

[Chris927]

Ah, yes, now I understand your concern :) I agree, making all sync resolvers magically async (by using a threadpool) is not a good idea. Maybe apply it to the top-level queries and mutations only?

[patrick91]

The issue with making sync resolvers magically async is that graphql potentially would have to resolve thousands of them per single request 🤔, that's why I think it's inefficient

yes, and the current implementation of gather is quite slow for function that are not async (might be better in 3.12 or 3.13 though)

Also, every single field is a resolver, for example:

query {
  user { # this is a call to a resolver
     field_a # also this
     field_b # and this one
  }
}

Maybe apply it to the top-level queries and mutations only?

I think I'd prefer to recommend to users to use async when possible more than us using threadpools, so that they can also leverage things like dataloaders

Also I wonder if we should extend your benchmarks to use gunicorn -k uvicorn.workers.UvicornWorker?

[ThirVondukr]

@patrick91 Same behavior:

@strawberry.type
class Query:
    @strawberry.field
    def hello(self) -> str:
        time.sleep(1)
        return "Hello World"

doctor@main MINGW64 /c/dev/python/sandbox
$ ./hey -c 100 -z 10s -D body.txt -m POST -T "application/json" http://127.0.0.1:8000/graphql

Summary:
  Total:        23.9953 secs
  Slowest:      19.0458 secs
  Fastest:      1.0125 secs
  Average:      10.0322 secs
  Requests/sec: 4.5426

  Total data:   646 bytes
  Size/request: 34 bytes

Response time histogram:
  1.013 [1]     |■■■■■■■■■■■■■■■■■■■■
  2.816 [1]     |■■■■■■■■■■■■■■■■■■■■
  4.619 [2]     |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  6.423 [2]     |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  8.226 [2]     |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  10.029 [1]    |■■■■■■■■■■■■■■■■■■■■
  11.832 [2]    |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  13.636 [2]    |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  15.439 [2]    |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  17.242 [2]    |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  19.046 [2]    |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■


Latency distribution:
  10% in 3.0163 secs
  25% in 6.0296 secs
  50% in 11.0347 secs
  75% in 16.0415 secs
  90% in 19.0458 secs
  0% in 0.0000 secs
  0% in 0.0000 secs

Details (average, fastest, slowest):
  DNS+dialup:   0.0010 secs, 1.0125 secs, 19.0458 secs
  DNS-lookup:   0.0000 secs, 0.0000 secs, 0.0000 secs
  req write:    0.0002 secs, 0.0000 secs, 0.0005 secs
  resp wait:    10.0309 secs, 1.0117 secs, 19.0439 secs
  resp read:    0.0000 secs, 0.0000 secs, 0.0001 secs

Status code distribution:
  [200] 19 responses

Error distribution:
  [6]   Post "http://127.0.0.1:8000/graphql": EOF
  [84]  Post "http://127.0.0.1:8000/graphql": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

[patrick91]

interesting, I think it might be also worth doing tests with a database 😊

We should probably have a test suite somewhere for this, what do you think?

[ThirVondukr]

If you're using synchronous IO it wouldn't matter if it's time.sleep or a db query, result should be the same.

[Aponace]

I am having the same issue with database calls. In this case I either resolve the whole database model with joins from top level (loosing the dynamic schema resolution in a nutshell) or wrap every function that has a database call with @make_async (like @Chris927 advised).

As a suggestion, maybe alongside strawberry.field add another wrapper called strawberry.async_field (with builtin threadpool execution), this way the user won't have to execute everything in thread and they can choose what they want to do.

[ThirVondukr]

@Aponace Can you use async resolvers instead? 🤔

[Aponace]

@Aponace Can you use async resolvers instead? 🤔

No quite possible (for me) as the database handler is being used by hundreds of functions outside of strawberry. That would require me to write another handler in async, which would be a duplicate of the sync one, then adjust the code everywhere and make sure everyone use the correct resolver in the correct context (would be a nightmare). In terms of time and effort the easiest path is to annotate with @make_async (which solved the issue for me).

If someone is writing the code from scratch and they are aware of this issue, they can write an async db handler. But, I think most of the people will write their code synchronous and then, when they hit performance issues, will discover this thread (same as me ~2 years into development).

[Mark90]

If you're using synchronous IO it wouldn't matter if it's time.sleep or a db query, result should be the same.

I've indeed had similar results in tests with select pg_sleep(0.1) postgres queries and tests with time.sleep(0.1) commands.

If someone is writing the code from scratch and they are aware of this issue, they can write an async db handler. But, I think most of the people will write their code synchronous and then, when they hit performance issues, will discover this thread (same as me ~2 years into development).

Same situation here. Due to the luxury of being able to scale out through k8s I hadn't really dived into our mediocre graphql query performance until last week. Large part of it is of our own doing - performing sync I/O within async resolvers. And indeed we also have sync resolvers, as mixing sync and async is documented without the caveat that sync resolvers block the event loop. While I can understand the reasoning behind it, it did surprise me.

Of course the best solution is to go fully async, but we're in a similar place where the sync database handler is used in many other places and refactoring/separating that is going to take a lot of work. I'll probably look at a similar make_async workaround for the top-level Query resolvers.

[jacob-retail-aware]

@Chris927 your make_async wrapper worked for my Strawberry FastAPI implementation. Thanks a mil

[aiguofer]

Similar situation here as @Mark90. We've been scaling k8s pods and hadn't really noticed just how bad each pod was at handling concurrency. It was by digging into some issues with our healthchecks that eventually discovered that all of our sync GQL mutations were completely blocking and FastAPI couldn't even handle a concurrent health check whether we implemented the endpoint using async or not.

Agreed some of the issues are on our side, but this really should, at the very least, be mentioned in the docs. I looked all over the Strawberry/FastAPI docs regarding how sync or async endpoints behave and found nothing. I had to spend hours adding sleep statements in different places and running concurrent requests to figure out this behavior, which then led me to this issue.

Any FastAPI user would assume that the behavior is similar to FastAPI: if the function is async it gets executed in the event loop, if it's not it gets executed in the threadpool. If behavior does not meet reasonable assumptions, it should be documented.

[patrick91]

@aiguofer would you mind sending a PR to explain this? 😊