Skip to content

Cancel remaining fields on exceptions #238

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

Merged
merged 1 commit into from
May 20, 2025

Conversation

mgorven
Copy link
Contributor

@mgorven mgorven commented May 13, 2025

gather() returns when the first exception is raised, but does not cancel any remaining tasks. These continue to run which is inefficient, and can also cause problems if they access shared resources like database connections.

Fixes: #236

@Cito
Copy link
Member

Cito commented May 13, 2025

Thank you @mgorven - will have a look later this week or on the weekend.

Copy link

codspeed-hq bot commented May 13, 2025

CodSpeed Performance Report

Merging #238 will not alter performance

Comparing mgorven:cancel-tasks (2821841) with main (0107e30)

Summary

✅ 14 untouched benchmarks

@mgorven mgorven force-pushed the cancel-tasks branch 2 times, most recently from 322f5f6 to 6456782 Compare May 13, 2025 23:23
@mgorven
Copy link
Contributor Author

mgorven commented May 14, 2025

Check is failing because coverage is not 100%, but this is because completed = True in the test case is not executed which is exactly what we're testing for.

Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this contribution and sorry for not looking into it earlier.

Your approach looks good to me, just a few suggestions.

Also, I think we need to do something similar in other places where we gather results asynchronously, e.g. in complete_list_value(). Let me know if you want to add it to this PR or create a separate one, or whether I should work on this.

gather() returns when the first exception is raised, but does not cancel
any remaining tasks. These continue to run which is inefficient, and can
also cause problems if they access shared resources like database
connections.

Fixes: graphql-python#236
@mgorven
Copy link
Contributor Author

mgorven commented May 19, 2025

Thanks for the review, I've addressed the comments. You can take the other instances, unless you aren't able to get to that for a while in which case I can help.

@Cito Cito merged commit 57c083d into graphql-python:main May 20, 2025
12 checks passed
@Cito
Copy link
Member

Cito commented May 20, 2025

Ok, will try to do something similar for the other instances as well and then create a release in the coming days.

@Cito
Copy link
Member

Cito commented May 20, 2025

Thank you again for your contribution to this project.

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.

RuntimeWarning: coroutine ... never awaited when a sibling resolver raises an error
3 participants