Support for Async Django

[jaw9c]

Feedback requested.

This PR adds support for Django async views. The general strategy follows the same principal as the base graphene library when supporting async. It implements the following things:

• Django based resolvers now check if they are being executed in an async thread and if so wrap any ORM queries in sync_to_async
• Resolvers now follow the same principals as the base graphene lib to handle resolving async and sync agnostically.
• A new async view - credit to this PR

The current progress is as followed:

• Get the library working for all custom fields in graphene-django when running under async
• Duplicate the existing tests and edit them to have an async version

Other thoughts:

• It's annoying to wrap all resolvers in sync_to_async if they are performing ORM ops. It would be nice to detect this error and throw a warning on existing codebases.

[jaw9c]

Progress update: All the fields resolvers are supporting both being executed in the async context and wrapping django sync code into a sync context. I've made the assumption that any resolving of DjangoXType/Field fields will need to be executed in a sync context which I think is pretty safe. I've started running through all the tests and adding an extra assertion that running in an async executor produces the same result as the sync one.

Currently people switching to using the async view will need to wrap all their top level resolvers (those that are directly on the Query class passed into the schema) in @sync_to_async if they run sync code. If someone has an idea of how to prevent this that would be great, but can't seem to wrap my head around that!

Next steps are to run through all the test files and adding the helper.

It would great if people could test this either on their production projects (speciically when running under ASGI), as 'm interested in how swapping in/out of the sync context affects the performance of the execution.

[firaskafri]

Progress update: All the fields resolvers are supporting both being executed in the async context and wrapping django sync code into a sync context. I've made the assumption that any resolving of DjangoXType/Field fields will need to be executed in a sync context which I think is pretty safe. I've started running through all the tests and adding an extra assertion that running in an async executor produces the same result as the sync one.

Currently people switching to using the async view will need to wrap all their top level resolvers (those that are directly on the Query class passed into the schema) in @sync_to_async if they run sync code. If someone has an idea of how to prevent this that would be great, but can't seem to wrap my head around that!

Next steps are to run through all the test files and adding the helper.

It would great if people could test this either on their production projects (speciically when running under ASGI), as 'm interested in how swapping in/out of the sync context affects the performance of the execution.

This is great news! Would discuss with my team to try this on production once its ready!

[firaskafri]

@superlevure i think it is good to go as soon as we clarify the docs
What do you think @kiendang @jaw9c @sjdemartini

[superlevure]

@dima-kov I had a look at your benchmarks and I have few remarks.

First, it looks like you're comparing the sync and async resolvers on the same branch of graphene-django (this PR's branch). I believe it would actually be more fair to compare the async / sync versions of this branch and the sync version of graphene-django's main branch since this PR also affects sync resolvers and the point of the benchmarks is also to make sure no performance penalty is introduced to already existing code.

Second, I noticed the sync resolver is returning 500 objects while the async version is only returning 10 objects.

I took the liberty of pushing a PR to your repo that covers those point, as well as setting up a docker based environment and postgres as a DB to be closer to a real life use case.

I obtain the following results:

# Sync version [main]
Concurrency Level:      100
Time taken for tests:   45.517 seconds
Complete requests:      1000
Failed requests:        0
Non-2xx responses:      1000
Total transferred:      60232000 bytes
Total body sent:        238000
HTML transferred:       59963000 bytes
Requests per second:    21.97 [#/sec] (mean)
Time per request:       4551.739 [ms] (mean)
Time per request:       45.517 [ms] (mean, across all concurrent requests)

# Sync version [this branch]
Concurrency Level:      100
Time taken for tests:   203.300 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      45805000 bytes
Total body sent:        244000
HTML transferred:       45382000 bytes
Requests per second:    4.92 [#/sec] (mean)
Time per request:       20330.019 [ms] (mean)
Time per request:       203.300 [ms] (mean, across all concurrent requests)

# Async version [this branch]
# didn't run till the end, see below

Data tends to show a huge perf hit for the sync version of this PR. Concerning the async version, I run into the following DB error which which makes all requests fail until I restart the DB:

2024-02-11 16:31:39.310 UTC [4579] FATAL:  sorry, too many clients already

I suspect the async version is leaving DB connections opened somewhere (Postgres max_connections settings is set at 100 which is the concurrency level used in the benchmark)

I'll continue to play with the PR a bit tonight to make sure there's nothing wrong with my setup, but I'm curious if others can reproduce similar results.

[dima-kov]

@superlevure thank you for looking into it! I missed that part

[dima-kov]

regarding the results you've got: hmm, thats really strange. Both sync and async (100 requests concurrently and 1k requests in total) showed me +- 20s. 100+s is smth unbelievable for me.
Maybe the thing is with dockerization.

I'm playing with main version now.
Main:

Concurrency Level:      100
Time taken for tests:   23.706 seconds
Complete requests:      1000
Failed requests:        0

[dolgidmi]

Data tends to show a huge perf hit for the sync version of this PR. Concerning the async version, I run into the following DB error which which makes all requests fail until I restart the DB:

2024-02-11 16:31:39.310 UTC [4579] FATAL:  sorry, too many clients already

I suspect the async version is leaving DB connections opened somewhere (Postgres max_connections settings is set at 100 which is the concurrency level used in the benchmark)

Django currently doesn't support persistent connections for ASGI https://code.djangoproject.com/ticket/33497

[henadzit]

Thank you for all your work! How can we help to get it merged?

[dima-kov]

Data tends to show a huge perf hit for the sync version of this PR. Concerning the async version, I run into the following DB error which which makes all requests fail until I restart the DB:

2024-02-11 16:31:39.310 UTC [4579] FATAL:  sorry, too many clients already

I suspect the async version is leaving DB connections opened somewhere (Postgres max_connections settings is set at 100 which is the concurrency level used in the benchmark)

Django currently doesn't support persistent connections for ASGI https://code.djangoproject.com/ticket/33497

For those who use any external connection pooler like pgbouncer or AWS RDS Connection Pooling, this will not be a problem.

https://docs.djangoproject.com/en/5.1/releases/5.1/#postgresql-connection-pools

django 5.1 will support connection pool and this might be an opportunity to merge the PR

[pcraciunoiu]

I was getting an error that the view is not async on the latest Django release

Based on this, there must be an async method or the property returns false. As a quick fix, I pushed this classproperty to be true:

UpliftAgency@cc665df

Feel free to add that in.

[jaw9c]

Apologies for the radio silence on this thread. Appreciate the feedback and efforts from everyone on this PR - in particular the benchmarking.

A fair few months ago, we rolled this into production at my work, utilising the new AsyncGraphQLView but found that we very quickly maxed out the DB connection pool. Since then we had to put the efforts on ice in lieu of making sure the startup was default alive... We've been using this branch in production since my last post, server under ASGI, but not using the AsyncGraphQLView.

I think this is safe to merge, but with an experimental warning for those willing to test and use the AsyncGraphQLView. (Perhaps we rename it to experimental_AsyncGraphQLView` or log a warning? Just to make sure people understand the DB connections point!

I've been carefully watching the movements by the Django team on making the ORM fully async, this PR is the latest, which is really encouraging!

[henadzit]

django 5.1 will support connection pool and this might be an opportunity to merge the PR

Looks like Django 5.1 got released!

[chris-stetter]

Are there plans to publish a tagged release? Happy to support if anything is missing.

[superlevure]

[..]
I think this is safe to merge, but with an experimental warning for those willing to test and use the AsyncGraphQLView. (Perhaps we rename it to experimental_AsyncGraphQLView` or log a warning? Just to make sure people understand the DB connections point!

[..]

@jaw9c, do you think it's safe to merge even though my benchmarks showed this PR degrades performance for sync resolvers?

[vt-rc]

any luck with this?

[hunasdf]

Is there any update of this?

[androane]

Maybe I'm missing something, but what's the point of graphene version 3 as long as it is simply not working?! or at least data loaders are not working. Not even with Django 5.1

[ulgens]

@androane graphene-django project died long ago, but I wasn't able to give an EOL status to it when I was a maintainer. (then somebody revoked my rights without communicating first) This is not a maintained project, it's just a couple of people at the time didn't want to archive it. Occasional merges of a couple of small things here and there is not even remotely enough to keep an API-layer library modern.

I helped multiple startups to remove graphene from their stack, or replace with Strawberry. Trying to keep this tool alive was a lot of work even 5 years ago and today, it's generally "Oh we made a mistake, please save us!" level thing. It's amazing to see how much time and energy is wasted just because there is no clear "NOT MAINTAINED" warning in the place.

[androane]

@ulgens Thanks so much for your answer, that explains so much and confirms my hunches as well...

[ulgens]

Merging something of this importance into a dead project will only confuse people, and (considering the project's history and lack of latest activities) I don't think that people will be around to improve or fix it when it will be needed.

Keeping the PR open gives an impression that someday this will be a working part of graphene, so I guess the least I can do is to reject it.

[ethagnawl]

That's extremely useful context, @ulgens. Thanks.

[syrusakbary]

I've been pinged about this thread two times in the last week.
A bit of context, I created both the GraphQL-Python community and the Graphene and Graphene Django projects, although I've been a bit disconnected from it lately due to my duties and work at @wasmerio.

graphene-django project died long ago, but I wasn't able to give an EOL status to it when I was a maintainer
@ulgens all maintainers need to be aligned for deprecating the project, which was not the case. It's true that Graphene-Django has not been active as of lately, but it's still alive and will making lots of progress moving forward.

I'll be taking on ownership of graphene-django back to add support of the learnings on using it with dataloaders over the last years in @wasmerio (we are using it heavily). Stay tuned

[syrusakbary]

Also, @jaw9c thanks for your hard work on this PR. I have no idea why no one on the maintainers team decided to merge your PR, but I'll be reviewing it soon and hopefully merge.

As part of the team of maintainers of Graphene Django, my sincere apologies for taking so much time to review it.

[dima-kov]

@syrusakbary thank you!

Graphene-Django still falls back to sync behavior in key execution paths. Given posted async load tests, do you see a feasible roadmap for full async compatibility? Do we need to wait full async compatibility from django core (if this will be merged: django/django#18408)?

Happy to contribute if there’s a focused plan—what do you see as the major blockers?

[jaw9c]

Glad to have you back @syrusakbary! Thanks for your kind words - I think the main blocker became that the performance didn't increase due to the reasons outlined by @dima-kov above. The majority of people will be hitting the ORM during their queries, and running async with results in more (slightly more) computation from nested use of sync_to_async. There is still a net benefit if a workload is non ORM heavy - but otherwise we're waiting for django/django#18408 to land AFAIK for the major benefits to be realised

[syrusakbary]

I took a deeper look on the PR.
Few things:

• Middleware in GraphQL is very costly (it can slow down things heavily), so we should avoid it as much as possible (DjangoSyncRequiredMiddleware). We can simply wrap some of the resolvers with @sync_to_async if needed
• Data fetching could be done
• I'm working on graphql-server which adds support for tons of frameworks with Sync and Async views. Graphene-Django will be using this moving forward (as it adds compatibility with websockets/channels and more)

[syrusakbary]

The majority of people will be hitting the ORM during their queries, and running async with results in more (slightly more) computation from nested use of sync_to_async

Using async with Django Channels / ASGI improves things a tiiny bit (since they use the database_sync_to_async)

For non-asgi views performance will be on par (if no middleware is added).
I'd like to plan next steps better and then get back to you on this PR :)