-
-
Notifications
You must be signed in to change notification settings - Fork 749
Catch BaseExceptions from user tasks
#5997
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
Unit Test Results 12 files ±0 12 suites ±0 5h 53m 8s ⏱️ - 35m 4s For more details on these failures and errors, see this check. Results for commit 137bee4. ± Comparison against base commit 5c7d555. |
|
There are related failures |
|
I have some vague memory of being very careful to |
137bee4 to
1e3cec0
Compare
|
This passes besides The fundamental problem is that we don't distinguish between an async user task raising In the first case, the task should transition to I don't know how we'd distinguish this. I also don't think we care too much because async user tasks are not a very complete feature right now. We just need some choice to resolve the inconsistency. The easiest but least correct answer is "just let it be erred". I could imagine a |
|
Added a heuristic for detecting when we've cancelled user async tasks due to |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 35m 50s ⏱️ + 2m 2s For more details on these failures, see this check. Results for commit 0bcc3a3. ± Comparison against base commit 2311d4d. ♻️ This comment has been updated with latest results. |
| ) | ||
| return RescheduleEvent( | ||
| key=ts.key, stimulus_id=f"cancelled-by-worker-close-{time()}" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a cosmetic tweak to this.
Could you add a unit test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already covered by distributed/tests/test_worker.py::test_close_while_executing[False]. I could write another test for it, but it would look almost exactly the same.
The CancelledError when not closing case is also covered by test_base_exception_in_task.
crusaderky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
BaseExceptionin task leads to task never completing #5958pre-commit run --all-files