From 70733ab02c05a3c278277f31050c71e68221a8ad Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Mon, 24 Feb 2025 10:17:28 +0100 Subject: [PATCH 1/2] Stop raising exception when actor is stopped. BacgroundService raises BaseExceptionGroup. We can check if task was cancelled by checking if there are CancelledError in the list of exception. Signed-off-by: Elzbieta Kotulska --- RELEASE_NOTES.md | 2 ++ src/frequenz/sdk/actor/_run_utils.py | 17 ++++++++++------- tests/actor/test_actor.py | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index ab7664401..c3f1ad5e3 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -17,3 +17,5 @@ ## Bug Fixes - Fixed bug with formulas raising exception when stopped. + +- Fix a bug that raised `CancelledError` when actor was started with `frequenz.sdk.actor.run` and stopped. diff --git a/src/frequenz/sdk/actor/_run_utils.py b/src/frequenz/sdk/actor/_run_utils.py index 35d85fd54..362aef601 100644 --- a/src/frequenz/sdk/actor/_run_utils.py +++ b/src/frequenz/sdk/actor/_run_utils.py @@ -38,18 +38,21 @@ async def run(*actors: Actor) -> None: done_tasks, pending_tasks = await asyncio.wait( pending_tasks, return_when=asyncio.FIRST_COMPLETED ) - # This should always be only one task, but we handle many for extra safety for task in done_tasks: - # Cancellation needs to be checked first, otherwise the other methods - # could raise a CancelledError - if task.cancelled(): + # BackgroundService returns a BaseExceptionGroup containing multiple + # exceptions. The 'task.result()' statement raises these exceptions, + # and 'except*' is used to handle them as a group. If the task raises + # multiple different exceptions, 'except*' will be invoked multiple times, + # once for each exception group. + try: + task.result() + except* asyncio.CancelledError: _logger.info("Actor %s: Cancelled while running.", task.get_name()) - elif exception := task.exception(): - _logger.error( + except* BaseException: # pylint: disable=broad-exception-caught + _logger.exception( "Actor %s: Raised an exception while running.", task.get_name(), - exc_info=exception, ) else: _logger.info("Actor %s: Finished normally.", task.get_name()) diff --git a/tests/actor/test_actor.py b/tests/actor/test_actor.py index 96015e481..d29391aa7 100644 --- a/tests/actor/test_actor.py +++ b/tests/actor/test_actor.py @@ -390,6 +390,6 @@ async def cancel_actor() -> None: (*RUN_INFO, "Actor EchoActor[EchoActor]: Starting..."), (*ACTOR_INFO, "Actor EchoActor[EchoActor]: Started."), (*ACTOR_INFO, "Actor EchoActor[EchoActor]: Cancelled."), - (*RUN_ERROR, "Actor EchoActor[EchoActor]: Raised an exception while running."), + (*RUN_INFO, "Actor EchoActor[EchoActor]: Cancelled while running."), (*RUN_INFO, "All 1 actor(s) finished."), ] From eff167b1f076b52647c740568f800c4305a8c53e Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Wed, 26 Feb 2025 11:38:18 +0100 Subject: [PATCH 2/2] Stop catching BaseException in actor/_run_utils.py::run method CancellError is the only BaseException that needs to be catched. Signed-off-by: Elzbieta Kotulska --- RELEASE_NOTES.md | 2 + src/frequenz/sdk/actor/_run_utils.py | 2 +- tests/actor/test_actor.py | 60 ---------------------------- 3 files changed, 3 insertions(+), 61 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index c3f1ad5e3..02a8a5c72 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -19,3 +19,5 @@ - Fixed bug with formulas raising exception when stopped. - Fix a bug that raised `CancelledError` when actor was started with `frequenz.sdk.actor.run` and stopped. + +- Stop catching `BaseException` in `frequenz.sdk.actor.run`. Only `CancelledError` and `Exception` are caught now. \ No newline at end of file diff --git a/src/frequenz/sdk/actor/_run_utils.py b/src/frequenz/sdk/actor/_run_utils.py index 362aef601..f47163b7a 100644 --- a/src/frequenz/sdk/actor/_run_utils.py +++ b/src/frequenz/sdk/actor/_run_utils.py @@ -49,7 +49,7 @@ async def run(*actors: Actor) -> None: task.result() except* asyncio.CancelledError: _logger.info("Actor %s: Cancelled while running.", task.get_name()) - except* BaseException: # pylint: disable=broad-exception-caught + except* Exception: # pylint: disable=broad-exception-caught _logger.exception( "Actor %s: Raised an exception while running.", task.get_name(), diff --git a/tests/actor/test_actor.py b/tests/actor/test_actor.py index d29391aa7..05a9f5f26 100644 --- a/tests/actor/test_actor.py +++ b/tests/actor/test_actor.py @@ -78,31 +78,6 @@ async def _run(self) -> None: print(f"{self} done (should not happen)") -class RaiseBaseExceptionActor(BaseTestActor): - """A faulty actor that raises a BaseException as soon as it receives a message.""" - - def __init__( - self, - recv: Receiver[int], - ) -> None: - """Create an instance. - - Args: - recv: A channel receiver for int data. - """ - super().__init__(name="test") - self._recv = recv - - async def _run(self) -> None: - """Start the actor and crash upon receiving a message.""" - print(f"{self} started") - self.inc_restart_count() - async for _ in self._recv: - print(f"{self} is about to crash") - raise MyBaseException("This is a test") - print(f"{self} done (should not happen)") - - ACTOR_INFO = ("frequenz.sdk.actor._actor", 20) ACTOR_ERROR = ("frequenz.sdk.actor._actor", 40) RUN_INFO = ("frequenz.sdk.actor._run_utils", 20) @@ -309,41 +284,6 @@ async def test_does_not_restart_on_normal_exit( ] -async def test_does_not_restart_on_base_exception( - actor_auto_restart_once: None, # pylint: disable=unused-argument - caplog: pytest.LogCaptureFixture, -) -> None: - """Create a faulty actor and expect it not to restart because it raises a base exception.""" - caplog.set_level("DEBUG", logger="frequenz.sdk.actor._actor") - caplog.set_level("DEBUG", logger="frequenz.sdk.actor._run_utils") - - channel: Broadcast[int] = Broadcast(name="channel") - - actor = RaiseBaseExceptionActor(channel.new_receiver()) - - async with asyncio.timeout(1.0): - await channel.new_sender().send(1) - # We can't use pytest.raises() here because known BaseExceptions are handled - # specially by pytest. - try: - await run(actor) - except MyBaseException as error: - assert str(error) == "This is a test" - - assert BaseTestActor.restart_count == 0 - assert caplog.record_tuples == [ - (*RUN_INFO, "Starting 1 actor(s)..."), - (*RUN_INFO, "Actor RaiseBaseExceptionActor[test]: Starting..."), - (*ACTOR_INFO, "Actor RaiseBaseExceptionActor[test]: Started."), - (*ACTOR_ERROR, "Actor RaiseBaseExceptionActor[test]: Raised a BaseException."), - ( - *RUN_ERROR, - "Actor RaiseBaseExceptionActor[test]: Raised an exception while running.", - ), - (*RUN_INFO, "All 1 actor(s) finished."), - ] - - async def test_does_not_restart_if_cancelled( actor_auto_restart_once: None, # pylint: disable=unused-argument caplog: pytest.LogCaptureFixture,