strawberry-graphql / strawberry-sqlalchemy

A SQLAlchemy Integration for strawberry-graphql
MIT License
91 stars 26 forks source link

Support async sessions in strawberry_sqlalchemy_mapper #53

Closed mattalbr closed 1 year ago

mattalbr commented 1 year ago

I left a note in the loader's init, but to duplicate that here: Making blocking database calls from within an async function (the resolver) has potentially catastrophic performance implications. Not only will all field resolution be effectively serialized, any other coroutines waiting on the event loop (e.g. concurrent requests in a web server), will be blocked as well, grinding your entire service to a halt.

More discussion needs to happen about what we do with the sync bind.

botberry commented 1 year ago

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Adds support for async sessions. To use:

from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
from strawberry_sqlalchemy_mapper import StrawberrySQLAlchemyLoader

url = "postgresql://..."
engine = create_async_engine(url)
sessionmaker = async_sessionmaker(engine)

loader = StrawberrySQLAlchemyLoader(async_bind_factory=sessionmaker)
codspeed-hq[bot] commented 1 year ago

CodSpeed Performance Report

Merging #53 will not alter performance

Comparing mattalbr:async_session_factory (dfeef2f) with main (9746996)

Summary

✅ 1 untouched benchmarks

codecov-commenter commented 1 year ago

Codecov Report

Merging #53 (dfeef2f) into main (070ec9a) will increase coverage by 1.10%. Report is 3 commits behind head on main. The diff coverage is 93.93%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #53 +/- ## ========================================== + Coverage 77.37% 78.48% +1.10% ========================================== Files 10 10 Lines 725 781 +56 Branches 108 116 +8 ========================================== + Hits 561 613 +52 - Misses 135 137 +2 - Partials 29 31 +2 ```
patrick91 commented 1 year ago

I'd love to get Erik's opinion on this 😊

patrick91 commented 1 year ago

I'd love to get Erik's opinion on this 😊

mattalbr commented 1 year ago

However, we must ensure that we don't compromise synchronous support in the process. As previously discussed in another PR, the halting of the event loop executing the GQL Request in a WSGI Webserver isn't particularly concerning due to the presence of other worker threads.

Given that Strawberry caters to both ASGI and WSGI, it's essential for us to provide comprehensive support for both async and sync sessions throughout this library. While striving for this, it's critical to:

  • Assess the clarity and elegance of the dual support implementation.
  • Ensure that the resulting codebase remains maintainable.

It's important to acknowledge that there are genuine reasons why some might opt for a synchronous driver over an asynchronous one. These reasons remain relevant, even if there's a need to incorporate some asynchronous code, such as for the Dataloader pattern.

I completely agree on emphasizing the distinctions and considerations between sync and async. It's a crucial topic that users should be well-informed about. The best place for that is the documentation of this library. We should get that started soon 😊

Thanks for your thoughts Erik!

I'm still extremely skeptical of this sync bind, but there's nothing forcing us to add support for async and deprecate sync at the same time, so for now I got rid of the deprecation in this PR and we can take more time to discuss further and come to recommendations that we document (good call out on the documentation!)

Let's chat more when you and Patrick have time about the possibility of deprecating sync. For now, I updated the PR description and title.