Make Extension system async.

[greemo]

Calls to request_started and request_finished in ExtensionManager should be made with await..

I can implement this if there is no objection..

[rafalp]

Is this to run blocking IO like api calls or database queries?

[greemo]

It's to close the db session. I use sqlalchemy in Async mode. close() is an Async method, and I only want to close it once the request is complete 19 Nov 2021 23:19:57 Rafał Pitoń ***@***.***>:

…

Is this to run blocking IO like api calls or database queries? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[#710 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AABYFTT5Y4KHXNDJDOUU3H3UMY6GVANCNFSM5G2BEEXQ]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEQAAABECAYAAAA4E5OyAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAoSURBVHic7cEBAQAAAIIg/69uSEABAAAAAAAAAAAAAAAAAAAAAACPBkiEAAHzeIrHAAAAAElFTkSuQmCC###24x24:true###][Tracking image][https://github.com/notifications/beacon/AABYFTUUXE4HTB3Z2XBCWBTUMY6GVA5CNFSM5G2BEEX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHIHG3QA.gif]

[rafalp]

GraphQL extensions shouldn't manage database connection. This should happen outside of Ariadne, eg. in Starlette's startup and shutdown methods.

[greemo]

I'm not talking about the connection, I'm talking about the session. We should have a new session every request so the cache is flushed per request.

[rafalp]

This should be implemented as ASGI middleware and not GraphQL extension. GraphQL extensions should be used for things that really really are associated with query execution process itself, and nothing else.

[greemo]

Hmm, I see querying the DB as being definitely related to executing a query. It is a bit strange that I need to bury by db session in the Request object, accessing it via context["request"].state.dbsession, but I could always use a context generator to make it available directly in the context.

I think as Ariadne is supposed to be an async platform, the extension system should also be async, but feel free to close it if having them as async feels wrong to you...

[rafalp]

I'm not arguing against making extensions system async, I can see a point in that (which is why issue is left open). I'm arguing against using them for managing database sessions.

Long story short, we've never considered Ariadne a web framework. It's ASGI/WSGI apps work without one because it makes it very easy to get started with your own GraphQL API for learning/prototyping and also for running simple API's in production.

However we are seeing more and more people trying to use its extension points (like extension system, connect/disconnect events) to shove in extra logic there that could as easily be placed into lifecycle methods of Starlette or ASGI middleware. Trying to shove connections/auth/session management leads to "it almost works but if only you could do this little change" discussions like this one (or one in websocket on connect/disconenct hooks) because those are designed for different use cases and there's no guarantee those use-cases will overlap with yours.

You can heat up a food in dishwasher, but your kitchen likely supports better ways to do that sort of deal. ;)

[dacevedo12]

I'd find this useful to read stuff from the request in starlette, as the request.json() method is async

Or maybe pass the operation info to the extension so one could easily implement performance tracking extensions.
I'm currently working around this limtiation by setting what I need in a custom context value, then using it in the extension.

[rafalp]

We could add a check in Ariadne's ASGI server for request_started and request_finished to see if their result isawaitable and await them if thats the case. That way both sync and async would be supported.