Fix issue with actor restarting instead of stopping #1223
Conversation
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where actors would restart instead of stopping when exceptions occurred during cancellation. Key changes include the introduction of a new actor subclass that raises an exception during cancellation, updates to the actor’s internal logic to properly handle cancelled state, and a new test that verifies the correct stopping behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/actor/test_actor.py | New actor subclass and test to check actor stops on cancellation. |
| src/frequenz/sdk/actor/_actor.py | Updates in actor startup and exception handling during cancellation. |
| RELEASE_NOTES.md | Updated release notes to describe the bug fix. |
When an actor throws an exception during cancellation, it restarts rather than stopping as expected. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
fix_error_loop
branch
2 times, most recently
from
51c6e89 to
1045eea
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where an actor restarted instead of stopping when an exception was raised during cancellation. Key changes include the addition of a new test case in tests/actor/test_actor.py, an adjustment in cancellation and exception logging logic in src/frequenz/sdk/actor/_actor.py, and updated release notes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/actor/test_actor.py | Added a new test and a specialized actor (RaiseExceptionOnCancelActor) to validate proper stop behavior upon cancellation errors. |
| src/frequenz/sdk/actor/_actor.py | Introduced a cancellation flag, refined exception handling in the run loop, and updated the cancel method to prevent actor restart. |
| RELEASE_NOTES.md | Updated the bug fix section to reflect that actors now stop and surface the unhandled exception during cancellation. |
| name: The name of this background service. | ||
| """ | ||
| super().__init__(name=name) | ||
| self._is_cancelled = False |
There was a problem hiding this comment.
Can't we save the main actor task instead and use self._main_task.cancelled() as a single source of truth?
There was a problem hiding this comment.
Checked and in that case self._main_task.cancelled() returns False, because task was not cancelled successfully.
| except asyncio.CancelledError as exc: | ||
| raise RuntimeError("Actor should stop.") from exc |
There was a problem hiding this comment.
I'm not sure we need to support this, this feels wrong, if a task is cancelled, it should not raise some other exception. Cancellation should always be supported and do nothing if it was already cancelled.
There was a problem hiding this comment.
if a task is cancelled, it should not raise some other exception.
Yes but bugs happens and anything can raise exception. The problem is in this case we restart the actor and await actor.stop() never ends... :/
When an actor threw an exception during cancellation, it restarted rather than stopping as expected. Now it stops and raises an unhandled exception. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
fix_error_loop
branch
from
1045eea to
02e30d7
Compare
github-project-automation
bot
moved this from To do
to Review approved
in Python SDK Roadmap
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
3 participants
When an actor threw an exception during cancellation,
it restarted rather than stopping as expected.
Now it stops and raises and just prints exception.