Skip to content

extmod/asyncio: Support gather of tasks that finish early. #13474

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

Conversation

dpgeorge
Copy link
Member

Adds support to asyncio.gather() for the following cases:

  • one or more sub-tasks finish before the gather starts
  • one or more sub-tasks raise an exception before the gather starts

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Jan 19, 2024
@dpgeorge
Copy link
Member Author

@iabdalkader FYI

Copy link

github-actions bot commented Jan 19, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +64 +0.008% standard
      stm32:   +72 +0.018% PYBV10
     mimxrt:   +72 +0.020% TEENSY40
        rp2:   +72 +0.022% RPI_PICO
       samd:   +68 +0.026% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (51fbec2) 98.36% compared to head (49e78d4) 98.36%.

❗ Current head 49e78d4 differs from pull request most recent head 2ecbad4. Consider uploading reports for the commit 2ecbad4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13474   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files         159      159           
  Lines       21088    21088           
=======================================
  Hits        20743    20743           
  Misses        345      345           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dpgeorge dpgeorge force-pushed the extmod-asyncio-gather-support-finished-tasks branch from cc5d4e8 to 49e78d4 Compare January 20, 2024 01:52
@dpgeorge
Copy link
Member Author

I found and fixed an edge case where all sub-tasks finish early.

Adds support to asyncio.gather() for the case that one or more (or all)
sub-tasks finish and/or raise an exception before the gather starts.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the extmod-asyncio-gather-support-finished-tasks branch from 49e78d4 to 2ecbad4 Compare January 22, 2024 01:06
@dpgeorge dpgeorge merged commit 2ecbad4 into micropython:master Jan 22, 2024
@dpgeorge dpgeorge deleted the extmod-asyncio-gather-support-finished-tasks branch January 22, 2024 01:16
@iabdalkader
Copy link
Contributor

@dpgeorge Thank you very much for this much needed fix 🙏🏼 I had a chance to test this today, and even though it fixes the main issue for me (gather() raising "can't gather"), I still see the following output... Is this expected, should I just ignore it ?

Task exception wasn't retrieved
future: <Task> coro= <generator object 'conn_task' at 240437a0>
Traceback (most recent call last):
  File "asyncio/core.py", line 1, in run_until_complete
  File "ucloud.py", line 341, in conn_task
DoneException: 

Also it seems the task exception is now saved, and I can retrieve it after gather() raises with t.data, but could we also retrieve it with Task.exception() for more compatibility with CPython's Task ? https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.exception

# Save exception raised by the coro for later use.
t.data = exc

@dpgeorge
Copy link
Member Author

I still see the following output... Is this expected, should I just ignore it ?

Yes that's expected. Because of the difference in GC vs reference counting, MicroPython and CPython necessarily differ in when exceptions like this are printed. You can see in the test in this PR that those messages are sent to a dummy custom_handler() which just ignores them. That way the test runs the same under CPy and uPy.

Also it seems the task exception is now saved, and I can retrieve it after gather() raises with t.data

You should also be able to retrieve the exception from the return value of the gather() call. Is that enough? If not, why not?

Also it seems the task exception is now saved, and I can retrieve it after gather() raises with t.data

If the return value of gather() is not enough we could consider adding Task.exception(). See #13000.

@iabdalkader
Copy link
Contributor

You should also be able to retrieve the exception from the return value of the gather() call. Is that enough? If not, why not?

This is how I catch the task exception:

            task_except = None
            try:
                await asyncio.gather(*self.tasks.values(), return_exceptions=False)
            except Exception as e:
                task_except = e
# Then later
            for name in list(self.tasks):
                task = self.tasks[name]
                try:
                    if task.done():
                         if isinstance(task_except, DoneException)
                break   # Break after the first task is removed.

This counts on tasks being processed by gather() in order, i.e tasks that raise exceptions are returned in order (which is always true I think?). The only problem with this, is that I have to process/cleanup one task at a time, then call gather() again, but it's not a big issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants