Skip to content

Fix incorrectly setting cancelled futures#44

Merged
markedwards merged 3 commits intosyrusakbary:masterfrom
Hellzed:master
Feb 12, 2025
Merged

Fix incorrectly setting cancelled futures#44
markedwards merged 3 commits intosyrusakbary:masterfrom
Hellzed:master

Conversation

@Hellzed
Copy link
Copy Markdown

@Hellzed Hellzed commented Feb 12, 2025

Hi there,

I'm running aiodataloader in production, not in a GraphQL context, but in a concurrent message consumer. Some messages consumed in the same event loop iteration may refer to the same data, and an uncached DataLoader helps with the IO load.

When the connection to the message broker drops, the current message handling tasks will get cancelled.

This leads to this typical log message:

ERROR    asyncio:base_events.py:1820 Task exception was never retrieved
future: <Task finished name='Task-65' coro=<dispatch_queue_batch() done, defined at /home/loris/Code/aiodataloader-next/aiodataloader/__init__.py:247> exception=InvalidStateError('invalid state')>
Traceback (most recent call last):
  File "/home/loris/Code/aiodataloader-next/aiodataloader/__init__.py", line 300, in dispatch_queue_batch
    ql.future.set_result(value)
asyncio.exceptions.InvalidStateError: invalid state

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/loris/Code/aiodataloader-next/aiodataloader/__init__.py", line 303, in dispatch_queue_batch
    return failed_dispatch(loader, queue, e)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/loris/Code/aiodataloader-next/aiodataloader/__init__.py", line 315, in failed_dispatch
    ql.future.set_exception(error)
asyncio.exceptions.InvalidStateError: invalid state

It turns out aiodataloader is not checking if an asyncio.Future is not already cancelled before attempting to set it, which is against asyncio recommended practice.

This PR enforces this check for all asyncio.Future set by aiodataloader.

@Hellzed
Copy link
Copy Markdown
Author

Hellzed commented Feb 12, 2025

@syrusakbary There seems to be an issue with branch checks, I'm unsure what's he next step.

@markedwards
Copy link
Copy Markdown
Collaborator

markedwards commented Feb 12, 2025

This generally looks good. You have some type errors in test_aiodataloader.py you need to fix. I will do a PR to sort out the failing test runs and add a test run for newer python versions.

@Hellzed
Copy link
Copy Markdown
Author

Hellzed commented Feb 12, 2025

I've just fixed the type errors, let me know if there's anything else I can help with.

@markedwards
Copy link
Copy Markdown
Collaborator

I have updated GitHub actions, please update and re-trigger CI.

@markedwards markedwards merged commit 240011b into syrusakbary:master Feb 12, 2025
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.

2 participants