Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc/issue-76 #81

Merged
merged 3 commits into from Jun 26, 2020
Merged

doc/issue-76 #81

merged 3 commits into from Jun 26, 2020

Conversation

daveoconnor
Copy link

Pull request for connection _unsubscribe implementation based on tasks rather than trying to directly close the async generator directly, to fix changes made in python 3.8.

Information to do it this way gotten from: https://bugs.python.org/issue38559

This is way outside of my wheelhouse so if someone with asyncio experience can take a look and make sure this is the correct way to proceed that'd be good.

Thanks

@abusi
Copy link
Contributor

abusi commented May 5, 2020

Can you add a test-case? If you think it's possible, in the meantime, I'll check if it is still "working" with 3.6/3.7 versions of python.
Thanks for this PR.

@abusi
Copy link
Contributor

abusi commented May 5, 2020

closes #76

@abusi abusi linked an issue May 5, 2020 that may be closed by this pull request
@daveoconnor
Copy link
Author

I was planning to write a test case when I started looking at it but noticed there were no test cases for any of that file. It's a bit more of an undertaking to write one for that change and I don't think I'm the best equipped person to do it, not having enough low level asyncio experience to really write effective tests.

@abusi
Copy link
Contributor

abusi commented May 5, 2020

It's okay @daveoconnor, no problem, I'll do it later then.
You confirm that, for you, this PR is solving your bug?
I'm working on another issue in the TTFTT engine right now :/

@daveoconnor
Copy link
Author

Great!

Yes, this is solving my bug on Python 3.8, based on aiohttp-3.6.2 and tartiflette-1.2.0.

@daveoconnor
Copy link
Author

Any idea when the next version might be out?

@abusi
Copy link
Contributor

abusi commented May 15, 2020

Soon:tm:

Can you try:

with supress(cancellederror)
     await operation.aclose()

Instead of what you've proposed?

I don't undestand why we should create a task on anext, cancel it and then await it with supress(cancellederror)

@abusi
Copy link
Contributor

abusi commented May 15, 2020

That's why I need time to understand the problem. Sadly i'm swamped at work.

@daveoconnor
Copy link
Author

The reason I did it with a task was because according to the thread I linked to earlier, if I'm reading it correctly, there may be an issue with just calling aclose().

https://bugs.python.org/issue38559#msg355163.

Double checking the results and how it impacts this bug, aclose() under the suppress seems to work too. It's been a while now but if I remember correctly I'd tried that at the time and was seeing subscriptions continuing to run, but I can't reproduce that now.

@abusi
Copy link
Contributor

abusi commented Jun 26, 2020

#76 (comment) -> Okay, I haven't had any time to check on this. I'll belive y'll, i'll take some time to merge and deliver a new version sunday evening CEST. Thanks

@abusi abusi merged commit bff03fa into tartiflette:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with subscriptions on refreshing graphiql page
2 participants