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

Make the behavior consistent for asyncio and asynq: raised error. #136

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

SoulTch
Copy link
Contributor

@SoulTch SoulTch commented Mar 2, 2024

This PR addressed two behaviors:

  • Propagates raised errors to the caller so that the caller can catch the exception.
  • If an asynq function yields a sequence of tasks, cancels all other tasks if any of them fail.

@dkang-quora
Copy link
Contributor

asynq does not cancel other concurrent tasks.
If you gather two independent tasks like two memcached requests to two hosts, a failure from one should not cancel the other one.

import asyncio

from asynq import asynq

counter = 0

@asynq()
def f():
    global counter
    counter += 1
    if counter == 3:
        raise RuntimeError('error 3')


@asynq()
def g():
    print('<<')
    try:
        yield [f.asynq(), f.asynq(), f.asynq(), f.asynq(), f.asynq()]
    except Exception as e:
        print('except', e)
    print('>>')


g()
print('counter=',counter)
<<
except error 3
>>
counter= 5

@dkang-quora
Copy link
Contributor

Can you fix this with return_exceptions=True? See asyncio.gather(*aws, return_exceptions=False)

@dkang-quora
Copy link
Contributor

Wait, no. We want return_exceptions=False. The current code does not cancel on the first failure.

@SoulTch
Copy link
Contributor Author

SoulTch commented Mar 2, 2024

Oh, I naively thought asynq will cancel the remaining tasks. Thanks for checking this.
So, which behavior is preferred?

  • If any of task raised an error, return immediately. Other tasks will remain in the event loop.
  • Wait all tasks. If any of them raised an error, raise any (or we can choose which one to raise).

It seems second one is more close to existing asynq.

@SoulTch
Copy link
Contributor Author

SoulTch commented Mar 5, 2024

Changed to:

  • Wait all tasks even though some of them raised an error. If two or more errors were raised, raise any of them.

@dkang-quora dkang-quora merged commit 5c98489 into quora:master Mar 6, 2024
10 checks passed
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