How to do per-request cleanup/teardown of context data?
dkbarn opened this issue · 15 comments
I am using SQLAlchemy to interact with a database in my GraphQL server. The creation of an SQLAlchemy connection/session is happening in my context_factory function, which ensures I get a new session on each web request.
But I need to properly close and cleanup this connection at the end of each request.
Is there a hook provided for doing per-request cleanup of data stored in the context?
Briefly glancing through the source code, I'm not seeing an obvious way of doing this.
Suggestion: What about implementing the context_factory function as a @contextlib.asynccontextmanager and invoking it in a "async with" statement here: https://github.com/tartiflette/tartiflette-aiohttp/blob/master/tartiflette_aiohttp/_handler.py#L39
This would provide a way for developers to build up the context, yield it, and then clean it up all within their custom context_factory.
Yes why not, feel free to do PR, i'll gladly review it and merge it. Thanks for this idea.
Ok, I will take a stab at creating a PR with the proposed changes.
One question: How important is it that this library maintain compatibility with Python 3.6? The reason I ask is that contextlib.asynccontextmanager was introduced into the standard library in Python 3.7
https://docs.python.org/3/library/contextlib.html#contextlib.asynccontextmanager
To create an asynchronous context manager in Python 3.6 will probably require introducing a dependency onto the pypi package "async_generator"
https://stackoverflow.com/a/48800772/6686740
PR created. Please have a look and let me know what you think.
Sorry to reopen but can there be a documentation update for this? https://tartiflette.io/docs/next/api-aiohttp/getting-started
Venting: I'm gutted I didn't see this discussion sooner. I'd written a way of doing exactly this that was completely out of tartiflette (uses aiohttp middleware, with modular server support) and was publishing it to pypi when I saw this. Had been working on it a while too but hadn't had a chance to write tests to finish it.
The reason was even exactly the same, to have sqlalchemy sessions passed into the context that I could .remove() (along with other plugins I'd written).
Sorry for the multiple comments but now that I'm digging into this more I can't help but think this should have been a new API option rather than a change to an existing one. This is breaking all sorts of things for me, and probably anyone else using it the old way.
For my use case (also sqlalchemy - amongst others) to match this change I now try passing an asynccontextmanager but then when I 'with' it in a resolver it then breaks if I use a subquery so I'd need to do new requests rather than accessing the data directly via the 'parent' argument as before.
The removal of it allowing a callable has meant there's really no way to data in except as a async generator, where I was relying on it to take methods for a few plugins I had. Can this be reverted and added back with this as an additional API option alongside the previous one?
Speaking more generally changing APIs is usually best done by adding interfaces, not by breaking existing ones.
Hi, I'm not the maintainer of this package, just the person who requested the feature and submitted the PR for it. I don't want to speak on their behalf, just wanted to offer up my 2 cents until they're able to respond:
Sorry to reopen but can there be a documentation update for this? https://tartiflette.io/docs/next/api-aiohttp/getting-started
I agree the functionality should be documented. Note that context_factory is not documented at all - not in the page you linked or the README of this repo. So it's not like the existing documentation is now out-of-date or incorrect. But it would be nice for context_factory to be added to the documentation.
I can't help but think this should have been a new API option rather than a change to an existing one. This is breaking all sorts of things for me, and probably anyone else using it the old way.
My original PR maintained backwards compatibility with the old behaviour of context_factory as a function. The decision was made by the maintainers to break backwards compatibility because (1) context_factory is undocumented and therefore probably not heavily used, and (2) the new behaviour can be released using a major or minor version bump, allowing users reliant on the old behaviour to pin the version they're using so as not to break existing software.
For my use case (also sqlalchemy - amongst others) to match this change I now try passing an asynccontextmanager but then when I 'with' it in a resolver it then breaks if I use a subquery so I'd need to do new requests rather than accessing the data directly via the 'parent' argument as before.
I don't entirely follow what you're doing here (a code example might help clarify). Are you putting a "with" statement in your resolver? This happens for you, in the _handle_query function: https://github.com/jugdizh/tartiflette-aiohttp/blob/feature/issue-94-context-factory-manager/tartiflette_aiohttp/_handler.py#L41
What you receive in the resolver is what you're yielding in your context_factory.
The removal of it allowing a callable has meant there's really no way to data in except as a async generator, where I was relying on it to take methods for a few plugins I had.
Again a code example might help demonstrate what you mean here.
Note that context_factory is not documented at all - not in the page you linked or the README of this repo. So it's not like the existing documentation is now out-of-date or incorrect.
From that page:
executor_context: Context which will be passed to each resolver (as a dict). Very useful for passing handlers to services, functions or data that you want to use in your resolvers.
req: Request object from aiohttp
app: Application object from aiohttp
I'll agree it's out of date and a lot of the documentation really needs a look over, but that's where the open source aspect of this comes into play. We read the code and build to that when the documentation is lacking. That's no justification for replacing the old functionality and throwing those of us who have already built things to that API in the past under the bus when it could have been added as another option to register_graphql_handlers and handled separately.
It's bad form and honestly worries me about building against this framework if things can just be broken without any concern for backward compatibility and users, and that sucks because I really like tartiflette. It's hard to advocate for it anywhere if things are going to break without warning for no benefit (over adding additional options).
My original PR maintained backwards compatibility with the old behaviour of context_factory as a function.
Fair point, I see that now. @dbarn can you provide some input on if that change can be reverted and added as an additional option instead, or some other solution that doesn't break backwards compatibility?
(2) the new behaviour can be released using a major or minor version bump, allowing users reliant on the old behaviour to pin the version they're using so as not to break existing software.
That thinking is only going to lock people into old versions with a painful/non-existent upgrade path. That's not a good path to take, especially when other ways of adding functionality are available, like additional options to register_graphql_handlers.
I don't entirely follow what you're doing here ...
I tried a few different things, none of which worked, but I'll leave this off the discussion for now.
Sorry, I'm in a bit of an awkward position here because it wasn't my decision to break backwards compatibility. My original PR overloaded the existing "context_factory" kwarg to support types of both callable and contextmanager. The removal of backwards compatibility was done in a separate commit, so would be easy enough to revert. That's not my call to make though.
I don't agree that introducing this as a new option is a good way to handle the feature. To me that feels like going down the road of feature creep. You would now have separate kwargs "context_factory" and (for example) "context_factory_contextmanager" -- which are mutually exclusive arguments whose job is to supply a context to your resolvers, but in different ways. I wouldn't say that having multiple kwargs which compete with each other and don't make sense to supply together is a particularly good design.
I'm willing to help you reach a solution with what you're trying to do, but I'd really need to see code to do that. Based on some of the things you've said, I think there might be some confusion or a misunderstanding about how these pieces fits together.
Transitioning your code from context_factory as a callable to context_factory as a contextmanager is in fact trivial, and there is no loss of functionality. That's why I'm confused by phrases like "a painful/non-existent upgrade path" or that something you were doing before is now impossible as a contextmanager.
Yeah, I totally understand your position, and actually really wish that context managers were an option when I started working on a couple of things a few weeks ago. I'd have used them for what I wanted to do but ended up discussing it on discord and going a different route, with the context manager on the server middleware instead.
or that something you were doing before is now impossible as a contextmanager.
I was in the middle of typing a response with a basic test case of what was possible before that isn't now, when things started working again. Now it seems like it's only on subscriptions that I'm having problems (which I'll need to investigate and see if I can just workaround but I'll add the error below if only to prove I'm not insane!).
I don't know if pipenv got into some weird state but what wasn't working earlier on query resolvers is now working as it used to with 1.2.0... I don't even need to make it a contextmanager. A method passed to the context works as it used to where an hour ago it raised an exception)
I'm so so frustrated about this. Now I can't even reproduce what was happening. It makes me look like I'm losing my mind (and maybe I am)!
Subscription error:
Traceback (most recent call last):
File "/host/.local/share/virtualenvs/PROJECT-XkHUY7Ky/lib/python3.8/site-packages/aiohttp/web_protocol.py", line 418, in start
resp = await task
File "/host/.local/share/virtualenvs/PROJECT-XkHUY7Ky/lib/python3.8/site-packages/aiohttp/web_app.py", line 458, in _handle
resp = await handler(request)
File "/host/.local/share/virtualenvs/PROJECT-XkHUY7Ky/lib/python3.8/site-packages/aiohttp/web_middlewares.py", line 119, in impl
return await handler(request)
File "/var/www/PROJECT/tartiflette_request_context_hooks/middleware/aiohttp.py", line 13, in manager
return await handler(request)
File "/var/www/PROJECT/tartiflette_request_context_hooks/middleware/aiohttp.py", line 13, in manager
return await handler(request)
File "/host/.local/share/virtualenvs/PROJECT-XkHUY7Ky/lib/python3.8/site-packages/aiohttp/web_urldispatcher.py", line 158, in handler_wrapper
return await result
File "/host/.local/share/virtualenvs/PROJECT-XkHUY7Ky/lib/python3.8/site-packages/tartiflette_aiohttp/_subscription_ws_handler.py", line 265, in call
self._context = await self._context_factory(request)
TypeError: object _AsyncGeneratorContextManager can't be used in 'await' expression
I don't think you're losing your mind, that looks like a legitimate bug! I'll admit that I was not considering subscriptions at all in my PR and didn't realise that context_factory was referenced there. It's also a bit of code that falls outside of the test coverage, so it's very plausible that my PR broke it.
I'm hoping it will be a trivial fix - I will look into submitting a patch later this weekend when I have some time.
That'd be great, thanks. Yeah, subscriptions seem to be the slightly ignored step-child on this project.
Just to circle back, while I was getting this error earlier I put it on hold and figured it was a subscriptions-being-ignored bug that I could look at or ignore.
What I was talking about earlier was on mutations/queries. A similar message about requiring AsyncGeneratorContextManager but not working when I provided one (as I'd mentioned) hence my complaints about backwards compatibility getting broken. Now I've no idea why I was getting that. Only thing I can think of was that it must have been some environmental fubar.
Anyway, thanks again for looking into this.
@daveoconnor I'm really sorry it breaks things for you. Because it was not really documented, I really thought not a lot of people where using it, because it was not really "done" (that's why it was and still is undocumented). also simply applying @asynccontextmanager over the previous context_factory was an easy fix for 1.2.0 users.
@jugdizh came with a good idea with how to pursue this feature, I thought why not doing it, if it's working well, i'll document it to make it a little bit more official.
Now i'm not sure I understand your problem. I'll be really happy to help, and to produce a 1.3.1 which solves this.
@abusi Thanks, working through things a bit more I realised that the biggest breakage I was seeing has ... evaporated?
I've gone into it in a previous comment but it seems to be fine for me with 1.3.0 now for some reason, and my plugins are now working again - with the exception of the subscription error, which, while it'd be nice to have that fixed I figured that was more 'bug' than 'throwing out backward compatibility' so the concern has mostly gone away after that was confirmed by @jugdizh.
Also adding @dbarn so he can know, please ignore the earlier ping.
I kind of want to mention why I was bothered by this, partly just to show I'm not just a whiner, I'll contribute! :)
I had been in the process of packaging up the following package as well a package it depended on, so now things are working, this is why!
https://github.com/daveoconnor/tartiflette-request-sqlalchemy-session
Hi @jugdizh, just checking back in on this one.
Thanks!