Skip to content

fix: Prevent InvalidStateError when resolving canceled futures#2339

Merged
patrick91 merged 1 commit intostrawberry-graphql:mainfrom
bellini666:fix_dataloader
Nov 15, 2022
Merged

fix: Prevent InvalidStateError when resolving canceled futures#2339
patrick91 merged 1 commit intostrawberry-graphql:mainfrom
bellini666:fix_dataloader

Conversation

@bellini666
Copy link
Copy Markdown
Member

@bellini666 bellini666 commented Nov 14, 2022

I see this error from time to time in my sentry installation: https://sentry.2u.app.br/share/issue/2c7efa8ad15145dd862cd2a95e455c98/

@bellini666 bellini666 self-assigned this Nov 14, 2022
@bellini666 bellini666 requested a review from patrick91 November 14, 2022 16:04
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 14, 2022

Codecov Report

Merging #2339 (aff237d) into main (dae3195) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2339   +/-   ##
=======================================
  Coverage   98.00%   98.00%           
=======================================
  Files         164      164           
  Lines        6666     6668    +2     
  Branches     1265     1266    +1     
=======================================
+ Hits         6533     6535    +2     
  Misses         65       65           
  Partials       68       68           

@botberry
Copy link
Copy Markdown
Member

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release changes the dataloader batch resolution to avoid resolving
futures that were canceled, and also from reusing them from the cache.
Trying to resolve a future that was canceled would raise asyncio.InvalidStateError


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Thiago Bellini Ribeiro for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

Copy link
Copy Markdown
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Nice one, thank you!

Comment thread tests/test_dataloaders.py
Comment on lines -260 to +261
await c4 == 4.4
assert await c4 == 4.4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

woops :D

@patrick91 patrick91 merged commit 8b700c5 into strawberry-graphql:main Nov 15, 2022
@bellini666 bellini666 deleted the fix_dataloader branch November 15, 2022 13:20
ben-xo added a commit to mixcloud/strawberry that referenced this pull request Mar 10, 2026
The error path in `dispatch_batch` called `set_exception` on every
future in the batch without checking if any had already been cancelled.
When a client disconnects mid-request, some DataLoader futures get
cancelled by the event loop; if the batch load function then fails
(e.g. because the executor was torn down), the error handler would
crash with `InvalidStateError` trying to set an exception on the
cancelled futures.

The success path already had this guard (added in strawberry-graphql#2339) but the
error path was missed. This adds the same `cancelled()` check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ben-xo added a commit to mixcloud/strawberry that referenced this pull request Mar 10, 2026
The error path in `dispatch_batch` called `set_exception` on every
future in the batch without checking if any had already been cancelled.
When a client disconnects mid-request, some DataLoader futures get
cancelled by the event loop; if the batch load function then fails
(e.g. because the executor was torn down), the error handler would
crash with `InvalidStateError` trying to set an exception on the
cancelled futures.

The success path already had this guard (added in strawberry-graphql#2339) but the
error path was missed. This adds the same `cancelled()` check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ben-xo added a commit to mixcloud/strawberry that referenced this pull request Mar 10, 2026
The error path in `dispatch_batch` called `set_exception` on every
future in the batch without checking if any had already been cancelled.
When a client disconnects mid-request, some DataLoader futures get
cancelled by the event loop; if the batch load function then fails
(e.g. because the executor was torn down), the error handler would
crash with `InvalidStateError` trying to set an exception on the
cancelled futures.

The success path already had this guard (added in strawberry-graphql#2339) but the
error path was missed. This adds the same `cancelled()` check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ben-xo added a commit to mixcloud/strawberry that referenced this pull request Mar 10, 2026
The error path in `dispatch_batch` called `set_exception` on every
future in the batch without checking if any had already been cancelled.
When a client disconnects mid-request, some DataLoader futures get
cancelled by the event loop; if the batch load function then fails
(e.g. because the executor was torn down), the error handler would
crash with `InvalidStateError` trying to set an exception on the
cancelled futures.

The success path already had this guard (added in strawberry-graphql#2339) but the
error path was missed. This adds the same `cancelled()` check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ben-xo added a commit to mixcloud/strawberry that referenced this pull request Mar 10, 2026
The error path in `dispatch_batch` called `set_exception` on every
future in the batch without checking if any had already been cancelled.
When a client disconnects mid-request, some DataLoader futures get
cancelled by the event loop; if the batch load function then fails
(e.g. because the executor was torn down), the error handler would
crash with `InvalidStateError` trying to set an exception on the
cancelled futures.

The success path already had this guard (added in strawberry-graphql#2339) but the
error path was missed. This adds the same `cancelled()` check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

3 participants