From ed5bd1a42da42bb60da18a1b46da77e748a82ff0 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 03:41:24 -0400 Subject: [PATCH 01/45] Add failing test --- tests/worker/test_workflow.py | 67 ++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index f51f7721b..2dc13550b 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -8,6 +8,7 @@ import threading import typing import uuid +import warnings from abc import ABC, abstractmethod from contextlib import contextmanager from dataclasses import dataclass @@ -31,7 +32,7 @@ import pytest from google.protobuf.timestamp_pb2 import Timestamp -from typing_extensions import Protocol, runtime_checkable +from typing_extensions import Literal, Protocol, runtime_checkable import temporalio.worker from temporalio import activity, workflow @@ -5176,3 +5177,67 @@ async def test_workflow_current_update(client: Client, env: WorkflowEnvironment) assert {"update1", "update2", "update3", "update4", "update5"} == set( await handle.result() ) + + +@workflow.defn +class UnfinishedHandlersWorkflow: + def __init__(self): + self.started_handler = False + + @workflow.run + async def run(self) -> None: + await workflow.wait_condition(lambda: self.started_handler) + return None + + @workflow.update + async def my_update(self) -> None: + self.started_handler = True + await workflow.wait_condition(lambda: False) + raise AssertionError("Unreachable") + + @workflow.signal + async def my_signal(self) -> None: + self.started_handler = True + await workflow.wait_condition(lambda: False) + raise AssertionError("Unreachable") + + +async def test_unfinished_update_handlers_warning(client: Client): + await _test_unfinished_handlers_warning(client, "update") + + +async def test_unfinished_signal_handlers_warning(client: Client): + await _test_unfinished_handlers_warning(client, "signal") + + +async def _test_unfinished_handlers_warning( + client: Client, handler_type: Literal["update", "signal"] +): + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + handle = await client.start_workflow( + UnfinishedHandlersWorkflow.run, + id=f"wf-{uuid.uuid4()}", + task_queue=worker.task_queue, + ) + with pytest.warns( + RuntimeWarning, + match=f"Workflow finished while {handler_type} handlers are still running", + ): + if handler_type == "signal": + await asyncio.gather( + handle.signal(UnfinishedHandlersWorkflow.my_signal) + ) + # In the case of signal, we must wait here for the workflow to complete to guarantee + # that we'll capture the warning emitted by the worker. + await handle.result() + else: + with pytest.raises(RPCError) as err: + await asyncio.gather( + handle.execute_update( + UnfinishedHandlersWorkflow.my_update, id="update1" + ) + ) + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" in str(err.value).lower() + ) From f6752163ad8a58514c80a061f9749b01d31db32b Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 09:20:24 -0400 Subject: [PATCH 02/45] Refactor test --- tests/worker/test_workflow.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 2dc13550b..d08ce448e 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5203,26 +5203,25 @@ async def my_signal(self) -> None: async def test_unfinished_update_handlers_warning(client: Client): - await _test_unfinished_handlers_warning(client, "update") + warnings = await _get_warnings_from_unfinished_handlers_workflow(client, "update") + assert any(_is_unfinished_handler_warning(w, "update") for w in warnings) async def test_unfinished_signal_handlers_warning(client: Client): - await _test_unfinished_handlers_warning(client, "signal") + warnings = await _get_warnings_from_unfinished_handlers_workflow(client, "signal") + assert any(_is_unfinished_handler_warning(w, "signal") for w in warnings) -async def _test_unfinished_handlers_warning( +async def _get_warnings_from_unfinished_handlers_workflow( client: Client, handler_type: Literal["update", "signal"] -): +) -> pytest.WarningsRecorder: async with new_worker(client, UnfinishedHandlersWorkflow) as worker: handle = await client.start_workflow( UnfinishedHandlersWorkflow.run, id=f"wf-{uuid.uuid4()}", task_queue=worker.task_queue, ) - with pytest.warns( - RuntimeWarning, - match=f"Workflow finished while {handler_type} handlers are still running", - ): + with pytest.WarningsRecorder() as warnings: if handler_type == "signal": await asyncio.gather( handle.signal(UnfinishedHandlersWorkflow.my_signal) @@ -5241,3 +5240,13 @@ async def _test_unfinished_handlers_warning( err.value.status == RPCStatusCode.NOT_FOUND and "workflow execution already completed" in str(err.value).lower() ) + + return warnings + +def _is_unfinished_handler_warning( + warning: warnings.WarningMessage, handler_type: Literal["update", "signal"] +) -> bool: + expected_text = f"Workflow finished while {handler_type} handlers are still running" + return issubclass(warning.category, RuntimeWarning) and expected_text in str( + warning.message + ) From c2a1961b310312454045acf75444d0fe87469ae0 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 06:14:13 -0400 Subject: [PATCH 03/45] Implement warning on unfinished signals and updates --- temporalio/worker/_workflow_instance.py | 59 ++++++++++++++++++++++++- temporalio/workflow.py | 14 ++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 13923b2ba..4b22c4d96 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -16,6 +16,7 @@ from contextlib import contextmanager from dataclasses import dataclass from datetime import timedelta +from enum import Enum from typing import ( Any, Awaitable, @@ -77,6 +78,31 @@ # Set to true to log all cases where we're ignoring things during delete LOG_IGNORE_DURING_DELETE = False +UNFINISHED_UPDATE_HANDLERS_WARNING = """ +Workflow finished while update handlers are still running. This may have interrupted work that the +update handler was doing, and the client that sent the update will receive a 'workflow execution +already completed' RPCError instead of the update result. You can wait for all update and signal +handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. +Alternatively, if both you and the clients sending updates and signals are okay with interrupting +running handlers when the workflow finishes, and causing clients to receive errors, then you can +disable this warning via the update handler decorator: +`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +""".replace( + "\n", " " +).strip() + +UNFINISHED_SIGNAL_HANDLERS_WARNING = """ +Workflow finished while signal handlers are still running. This may have interrupted work that the +signal handler was doing. You can wait for all update and signal handlers to complete by using +`await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both +you and the clients sending updates and signals are okay with interrupting running handlers when the +workflow finishes, and causing clients to receive errors, then you can disable this warning via the +signal handler decorator: +`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +""".replace( + "\n", " " +).strip() + class WorkflowRunner(ABC): """Abstract runner for workflows that creates workflow instances to run. @@ -240,6 +266,18 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: self._queries = dict(self._defn.queries) self._updates = dict(self._defn.updates) + # We record in-progress signals and updates in order to support waiting for handlers to + # finish, and issuing warnings when the workflow exits with unfinished handlers. Since + # signals lack a unique per-invocation identifier, we introduce a sequence number for the + # purpose. + self._handled_signals_seq = 0 + self._in_progress_signals: dict[ + int, temporalio.bridge.proto.workflow_activation.SignalWorkflow + ] = {} + self._in_progress_updates: dict[ + str, temporalio.bridge.proto.workflow_activation.DoUpdate + ] = {} + # Add stack trace handler # TODO(cretz): Is it ok that this can be forcefully overridden by the # workflow author? They could technically override in interceptor @@ -412,6 +450,10 @@ def activate( continue i += 1 + if seen_completion and self._in_progress_updates: + warnings.warn(UNFINISHED_UPDATE_HANDLERS_WARNING, RuntimeWarning) + if seen_completion and self._in_progress_signals: + warnings.warn(UNFINISHED_SIGNAL_HANDLERS_WARNING, RuntimeWarning) return self._current_completion def _apply( @@ -483,6 +525,7 @@ async def run_update() -> None: command.update_response.protocol_instance_id = job.protocol_instance_id past_validation = False try: + self._in_progress_updates[job.id] = job defn = self._updates.get(job.name) or self._updates.get(None) if not defn: known_updates = sorted([k for k in self._updates.keys() if k]) @@ -572,6 +615,8 @@ async def run_update() -> None: ) return raise + finally: + del self._in_progress_updates[job.id] self.create_task( run_update(), @@ -1646,8 +1691,20 @@ def _process_signal_job( input = HandleSignalInput( signal=job.signal_name, args=args, headers=job.headers ) + + async def run_signal(): + try: + self._handled_signals_seq += 1 + id = self._handled_signals_seq + self._in_progress_signals[id] = job + await self._run_top_level_workflow_function( + self._inbound.handle_signal(input) + ) + finally: + del self._in_progress_signals[id] + self.create_task( - self._run_top_level_workflow_function(self._inbound.handle_signal(input)), + run_signal(), name=f"signal: {job.signal_name}", ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 64173266b..ecfffa0fa 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -173,6 +173,20 @@ def run(fn: CallableAsyncType) -> CallableAsyncType: return fn # type: ignore[return-value] +class UnfinishedHandlersPolicy(IntEnum): + """Actions taken if a workflow terminates with running handlers. + + Policy defining actions taken when a workflow exits while update or signal handlers are running. + The workflow exit may be due to successful return, failure, cancellation, or continue-as-new. + """ + + # Abandon the handler. In the case of an update handler this means that the client will receive + # an error rather than the update result. + ABANDON = 1 + # Issue a warning in addition to abandoning. + WARN_AND_ABANDON = 2 + + @overload def signal(fn: CallableSyncOrAsyncReturnNoneType) -> CallableSyncOrAsyncReturnNoneType: ... From 8945ea672d2fc26b8bebc4651316814e60fc7d33 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 09:03:21 -0400 Subject: [PATCH 04/45] Implement all_handlers_finished --- temporalio/worker/_workflow_instance.py | 3 +++ temporalio/workflow.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 4b22c4d96..257e0ffd2 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -1147,6 +1147,9 @@ def workflow_set_update_handler( else: self._updates.pop(name, None) + def workflow_all_handlers_finished(self) -> bool: + return not self._in_progress_updates and not self._in_progress_signals + def workflow_start_activity( self, activity: Any, diff --git a/temporalio/workflow.py b/temporalio/workflow.py index ecfffa0fa..2e1129186 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -598,6 +598,10 @@ def workflow_set_update_handler( ) -> None: ... + @abstractmethod + def workflow_all_handlers_finished(self) -> bool: + ... + @abstractmethod def workflow_start_activity( self, @@ -4414,6 +4418,19 @@ def set_dynamic_update_handler( _Runtime.current().workflow_set_update_handler(None, handler, validator) +def all_handlers_finished() -> bool: + """Whether update and signal handlers have finished executing. + + Consider waiting on this condition before workflow return or continue-as-new, to prevent + interruption of in-progress handlers by workflow exit: + ``await workflow.wait_condition(lambda: workflow.all_handlers_finished())`` + + Returns: + True if there are no in-progress update or signal handler executions. + """ + return _Runtime.current().workflow_all_handlers_finished() + + def as_completed( fs: Iterable[Awaitable[AnyType]], *, timeout: Optional[float] = None ) -> Iterator[Awaitable[AnyType]]: From 8a5f4322b6e91c323650a3220e9f3f9de6cfec2e Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 09:28:56 -0400 Subject: [PATCH 05/45] Test waiting for all_handlers_finished --- tests/worker/test_workflow.py | 59 +++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index d08ce448e..e1c937b09 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5183,41 +5183,57 @@ async def test_workflow_current_update(client: Client, env: WorkflowEnvironment) class UnfinishedHandlersWorkflow: def __init__(self): self.started_handler = False + self.handler_may_return = False @workflow.run - async def run(self) -> None: + async def run(self, wait_for_handlers: bool) -> None: await workflow.wait_condition(lambda: self.started_handler) - return None + if wait_for_handlers: + self.handler_may_return = True + await workflow.wait_condition(workflow.all_handlers_finished) @workflow.update async def my_update(self) -> None: self.started_handler = True - await workflow.wait_condition(lambda: False) - raise AssertionError("Unreachable") + await workflow.wait_condition(lambda: self.handler_may_return) @workflow.signal - async def my_signal(self) -> None: + async def my_signal(self): self.started_handler = True - await workflow.wait_condition(lambda: False) - raise AssertionError("Unreachable") + await workflow.wait_condition(lambda: self.handler_may_return) -async def test_unfinished_update_handlers_warning(client: Client): - warnings = await _get_warnings_from_unfinished_handlers_workflow(client, "update") +async def test_unfinished_update_handlers(client: Client): + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, handler_type="update", wait_for_handlers=False + ) assert any(_is_unfinished_handler_warning(w, "update") for w in warnings) + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, handler_type="update", wait_for_handlers=True + ) + assert not any(_is_unfinished_handler_warning(w, "update") for w in warnings) -async def test_unfinished_signal_handlers_warning(client: Client): - warnings = await _get_warnings_from_unfinished_handlers_workflow(client, "signal") +async def test_unfinished_signal_handlers(client: Client): + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, handler_type="signal", wait_for_handlers=False + ) assert any(_is_unfinished_handler_warning(w, "signal") for w in warnings) + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, handler_type="signal", wait_for_handlers=True + ) + assert not any(_is_unfinished_handler_warning(w, "signal") for w in warnings) async def _get_warnings_from_unfinished_handlers_workflow( - client: Client, handler_type: Literal["update", "signal"] + client: Client, + handler_type: Literal["update", "signal"], + wait_for_handlers: bool, ) -> pytest.WarningsRecorder: async with new_worker(client, UnfinishedHandlersWorkflow) as worker: handle = await client.start_workflow( UnfinishedHandlersWorkflow.run, + arg=wait_for_handlers, id=f"wf-{uuid.uuid4()}", task_queue=worker.task_queue, ) @@ -5230,19 +5246,28 @@ async def _get_warnings_from_unfinished_handlers_workflow( # that we'll capture the warning emitted by the worker. await handle.result() else: - with pytest.raises(RPCError) as err: + if not wait_for_handlers: + with pytest.raises(RPCError) as err: + await asyncio.gather( + handle.execute_update( + UnfinishedHandlersWorkflow.my_update, id="update1" + ) + ) + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" + in str(err.value).lower() + ) + else: await asyncio.gather( handle.execute_update( UnfinishedHandlersWorkflow.my_update, id="update1" ) ) - assert ( - err.value.status == RPCStatusCode.NOT_FOUND - and "workflow execution already completed" in str(err.value).lower() - ) return warnings + def _is_unfinished_handler_warning( warning: warnings.WarningMessage, handler_type: Literal["update", "signal"] ) -> bool: From 8a1c7e640f256934d46b7a78f1f3efc4a363c1b8 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 09:55:48 -0400 Subject: [PATCH 06/45] Run in same worker --- tests/worker/test_workflow.py | 91 +++++++++++++++++------------------ 1 file changed, 45 insertions(+), 46 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index e1c937b09..5b3593c2f 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5204,68 +5204,67 @@ async def my_signal(self): async def test_unfinished_update_handlers(client: Client): - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, handler_type="update", wait_for_handlers=False - ) - assert any(_is_unfinished_handler_warning(w, "update") for w in warnings) - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, handler_type="update", wait_for_handlers=True - ) - assert not any(_is_unfinished_handler_warning(w, "update") for w in warnings) + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, worker, handler_type="update", wait_for_handlers=False + ) + assert any(_is_unfinished_handler_warning(w, "update") for w in warnings) + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, worker, handler_type="update", wait_for_handlers=True + ) + assert not any(_is_unfinished_handler_warning(w, "update") for w in warnings) async def test_unfinished_signal_handlers(client: Client): - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, handler_type="signal", wait_for_handlers=False - ) - assert any(_is_unfinished_handler_warning(w, "signal") for w in warnings) - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, handler_type="signal", wait_for_handlers=True - ) - assert not any(_is_unfinished_handler_warning(w, "signal") for w in warnings) + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, worker, handler_type="signal", wait_for_handlers=False + ) + assert any(_is_unfinished_handler_warning(w, "signal") for w in warnings) + warnings = await _get_warnings_from_unfinished_handlers_workflow( + client, worker, handler_type="signal", wait_for_handlers=True + ) + assert not any(_is_unfinished_handler_warning(w, "signal") for w in warnings) async def _get_warnings_from_unfinished_handlers_workflow( client: Client, + worker: Worker, handler_type: Literal["update", "signal"], wait_for_handlers: bool, ) -> pytest.WarningsRecorder: - async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - handle = await client.start_workflow( - UnfinishedHandlersWorkflow.run, - arg=wait_for_handlers, - id=f"wf-{uuid.uuid4()}", - task_queue=worker.task_queue, - ) - with pytest.WarningsRecorder() as warnings: - if handler_type == "signal": - await asyncio.gather( - handle.signal(UnfinishedHandlersWorkflow.my_signal) - ) - # In the case of signal, we must wait here for the workflow to complete to guarantee - # that we'll capture the warning emitted by the worker. - await handle.result() - else: - if not wait_for_handlers: - with pytest.raises(RPCError) as err: - await asyncio.gather( - handle.execute_update( - UnfinishedHandlersWorkflow.my_update, id="update1" - ) - ) - assert ( - err.value.status == RPCStatusCode.NOT_FOUND - and "workflow execution already completed" - in str(err.value).lower() - ) - else: + handle = await client.start_workflow( + UnfinishedHandlersWorkflow.run, + arg=wait_for_handlers, + id=f"wf-{uuid.uuid4()}", + task_queue=worker.task_queue, + ) + with pytest.WarningsRecorder() as warnings: + if handler_type == "signal": + await asyncio.gather(handle.signal(UnfinishedHandlersWorkflow.my_signal)) + # In the case of signal, we must wait here for the workflow to complete to guarantee + # that we'll capture the warning emitted by the worker. + await handle.result() + else: + if not wait_for_handlers: + with pytest.raises(RPCError) as err: await asyncio.gather( handle.execute_update( UnfinishedHandlersWorkflow.my_update, id="update1" ) ) + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" in str(err.value).lower() + ) + else: + await asyncio.gather( + handle.execute_update( + UnfinishedHandlersWorkflow.my_update, id="update1" + ) + ) - return warnings + return warnings def _is_unfinished_handler_warning( From 8eef8a6d1c53d75ea2c2c9c7f5c0de4b99044b47 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 10:02:04 -0400 Subject: [PATCH 07/45] Refactor --- tests/worker/test_workflow.py | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 5b3593c2f..aa30c8afc 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -14,6 +14,7 @@ from dataclasses import dataclass from datetime import datetime, timedelta, timezone from enum import IntEnum +from functools import partial from typing import ( Any, Awaitable, @@ -5204,27 +5205,31 @@ async def my_signal(self): async def test_unfinished_update_handlers(client: Client): - async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, worker, handler_type="update", wait_for_handlers=False - ) - assert any(_is_unfinished_handler_warning(w, "update") for w in warnings) - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, worker, handler_type="update", wait_for_handlers=True - ) - assert not any(_is_unfinished_handler_warning(w, "update") for w in warnings) + await _test_unfinished_handlers(client, "update") async def test_unfinished_signal_handlers(client: Client): + await _test_unfinished_handlers(client, "signal") + + +async def _test_unfinished_handlers( + client: Client, handler_type: Literal["update", "signal"] +): async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, worker, handler_type="signal", wait_for_handlers=False + get_warnings = partial( + _get_warnings_from_unfinished_handlers_workflow, + client, + worker, + handler_type, + ) + assert any( + _is_unfinished_handler_warning(w, handler_type) + for w in await get_warnings(wait_for_handlers=False) ) - assert any(_is_unfinished_handler_warning(w, "signal") for w in warnings) - warnings = await _get_warnings_from_unfinished_handlers_workflow( - client, worker, handler_type="signal", wait_for_handlers=True + assert not any( + _is_unfinished_handler_warning(w, handler_type) + for w in await get_warnings(wait_for_handlers=True) ) - assert not any(_is_unfinished_handler_warning(w, "signal") for w in warnings) async def _get_warnings_from_unfinished_handlers_workflow( From ee4fe48b41cadc0e0eb9028498d40944e154e4bd Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 10:10:27 -0400 Subject: [PATCH 08/45] Refactor test --- tests/worker/test_workflow.py | 126 ++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 60 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index aa30c8afc..965a9dfbd 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5205,77 +5205,83 @@ async def my_signal(self): async def test_unfinished_update_handlers(client: Client): - await _test_unfinished_handlers(client, "update") + await _UnfinishedHandlersTest(client, "update").run_test() async def test_unfinished_signal_handlers(client: Client): - await _test_unfinished_handlers(client, "signal") + await _UnfinishedHandlersTest(client, "signal").run_test() -async def _test_unfinished_handlers( - client: Client, handler_type: Literal["update", "signal"] -): - async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - get_warnings = partial( - _get_warnings_from_unfinished_handlers_workflow, - client, - worker, - handler_type, - ) - assert any( - _is_unfinished_handler_warning(w, handler_type) - for w in await get_warnings(wait_for_handlers=False) - ) - assert not any( - _is_unfinished_handler_warning(w, handler_type) - for w in await get_warnings(wait_for_handlers=True) - ) +@dataclass +class _UnfinishedHandlersTest: + client: Client + handler_type: Literal["update", "signal"] + async def run_test(self): + async with new_worker(self.client, UnfinishedHandlersWorkflow) as worker: + # The unfinished handler warning is issued by default, + assert any( + self.is_unfinished_handler_warning(w) + for w in await self.capture_warnings_from_workflow( + worker, wait_for_handlers=False + ) + ) + # but not when the workflow waits for handlers to complete. + assert not any( + self.is_unfinished_handler_warning(w) + for w in await self.capture_warnings_from_workflow( + worker, wait_for_handlers=True + ) + ) -async def _get_warnings_from_unfinished_handlers_workflow( - client: Client, - worker: Worker, - handler_type: Literal["update", "signal"], - wait_for_handlers: bool, -) -> pytest.WarningsRecorder: - handle = await client.start_workflow( - UnfinishedHandlersWorkflow.run, - arg=wait_for_handlers, - id=f"wf-{uuid.uuid4()}", - task_queue=worker.task_queue, - ) - with pytest.WarningsRecorder() as warnings: - if handler_type == "signal": - await asyncio.gather(handle.signal(UnfinishedHandlersWorkflow.my_signal)) - # In the case of signal, we must wait here for the workflow to complete to guarantee - # that we'll capture the warning emitted by the worker. - await handle.result() - else: - if not wait_for_handlers: - with pytest.raises(RPCError) as err: + async def capture_warnings_from_workflow( + self, + worker: Worker, + wait_for_handlers: bool, + ) -> pytest.WarningsRecorder: + handle = await self.client.start_workflow( + UnfinishedHandlersWorkflow.run, + arg=wait_for_handlers, + id=f"wf-{uuid.uuid4()}", + task_queue=worker.task_queue, + ) + with pytest.WarningsRecorder() as warnings: + if self.handler_type == "signal": + await asyncio.gather( + handle.signal(UnfinishedHandlersWorkflow.my_signal) + ) + # In the case of signal, we must wait here for the workflow to complete to guarantee + # that we'll capture the warning emitted by the worker. + await handle.result() + else: + if not wait_for_handlers: + with pytest.raises(RPCError) as err: + await asyncio.gather( + handle.execute_update( + UnfinishedHandlersWorkflow.my_update, id="update1" + ) + ) + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" + in str(err.value).lower() + ) + else: await asyncio.gather( handle.execute_update( UnfinishedHandlersWorkflow.my_update, id="update1" ) ) - assert ( - err.value.status == RPCStatusCode.NOT_FOUND - and "workflow execution already completed" in str(err.value).lower() - ) - else: - await asyncio.gather( - handle.execute_update( - UnfinishedHandlersWorkflow.my_update, id="update1" - ) - ) - return warnings + return warnings - -def _is_unfinished_handler_warning( - warning: warnings.WarningMessage, handler_type: Literal["update", "signal"] -) -> bool: - expected_text = f"Workflow finished while {handler_type} handlers are still running" - return issubclass(warning.category, RuntimeWarning) and expected_text in str( - warning.message - ) + def is_unfinished_handler_warning( + self, + warning: warnings.WarningMessage, + ) -> bool: + expected_text = ( + f"Workflow finished while {self.handler_type} handlers are still running" + ) + return issubclass(warning.category, RuntimeWarning) and expected_text in str( + warning.message + ) From 4c6bb27f609674731e3cc012f0e80dd28a005e93 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 10:39:23 -0400 Subject: [PATCH 09/45] Store policy with in-progress job --- temporalio/worker/_workflow_instance.py | 34 +++++++++++++++++++------ temporalio/workflow.py | 12 +++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 257e0ffd2..a22ef8e8d 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -272,10 +272,18 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: # purpose. self._handled_signals_seq = 0 self._in_progress_signals: dict[ - int, temporalio.bridge.proto.workflow_activation.SignalWorkflow + int, + Tuple[ + temporalio.bridge.proto.workflow_activation.SignalWorkflow, + temporalio.workflow.UnfinishedHandlersPolicy, + ], ] = {} self._in_progress_updates: dict[ - str, temporalio.bridge.proto.workflow_activation.DoUpdate + str, + Tuple[ + temporalio.bridge.proto.workflow_activation.DoUpdate, + temporalio.workflow.UnfinishedHandlersPolicy, + ], ] = {} # Add stack trace handler @@ -450,10 +458,17 @@ def activate( continue i += 1 - if seen_completion and self._in_progress_updates: - warnings.warn(UNFINISHED_UPDATE_HANDLERS_WARNING, RuntimeWarning) - if seen_completion and self._in_progress_signals: - warnings.warn(UNFINISHED_SIGNAL_HANDLERS_WARNING, RuntimeWarning) + if seen_completion: + if any( + policy == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + for _, policy in self._in_progress_updates.values() + ): + warnings.warn(UNFINISHED_UPDATE_HANDLERS_WARNING, RuntimeWarning) + if any( + policy == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + for _, policy in self._in_progress_signals.values() + ): + warnings.warn(UNFINISHED_SIGNAL_HANDLERS_WARNING, RuntimeWarning) return self._current_completion def _apply( @@ -525,7 +540,6 @@ async def run_update() -> None: command.update_response.protocol_instance_id = job.protocol_instance_id past_validation = False try: - self._in_progress_updates[job.id] = job defn = self._updates.get(job.name) or self._updates.get(None) if not defn: known_updates = sorted([k for k in self._updates.keys() if k]) @@ -533,6 +547,10 @@ async def run_update() -> None: f"Update handler for '{job.name}' expected but not found, and there is no dynamic handler. " f"known updates: [{' '.join(known_updates)}]" ) + self._in_progress_updates[job.id] = ( + job, + defn.unfinished_handlers_policy, + ) args = self._process_handler_args( job.name, job.input, @@ -1699,7 +1717,7 @@ async def run_signal(): try: self._handled_signals_seq += 1 id = self._handled_signals_seq - self._in_progress_signals[id] = job + self._in_progress_signals[id] = (job, defn.unfinished_handlers_policy) await self._run_top_level_workflow_function( self._inbound.handle_signal(input) ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 2e1129186..a976ea75f 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -211,6 +211,7 @@ def signal( *, name: Optional[str] = None, dynamic: Optional[bool] = False, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow signal method. @@ -231,6 +232,8 @@ def signal( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. + unfinished_handlers_policy: Actions taken if a workflow terminates with + a running instance of this handler. """ def with_name( @@ -964,6 +967,7 @@ def update( *, name: Optional[str] = None, dynamic: Optional[bool] = False, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow update handler method. @@ -991,6 +995,8 @@ def update( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. + unfinished_handlers_policy: Actions taken if a workflow terminates with + a running instance of this handler. """ def with_name( @@ -1468,6 +1474,9 @@ class _SignalDefinition: name: Optional[str] fn: Callable[..., Union[None, Awaitable[None]]] is_method: bool + unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + UnfinishedHandlersPolicy.WARN_AND_ABANDON + ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None dynamic_vararg: bool = False @@ -1549,6 +1558,9 @@ class _UpdateDefinition: name: Optional[str] fn: Callable[..., Union[Any, Awaitable[Any]]] is_method: bool + unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + UnfinishedHandlersPolicy.WARN_AND_ABANDON + ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None ret_type: Optional[Type] = None From 916cd7224c457f3a4b5139a843cd3271c39d6dbc Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 11:11:55 -0400 Subject: [PATCH 10/45] Add new parameter to signal and update decorators --- temporalio/worker/_workflow_instance.py | 4 +- temporalio/workflow.py | 99 +++++++++++++++++++++---- tests/worker/test_workflow.py | 77 ++++++++++++++----- 3 files changed, 145 insertions(+), 35 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index a22ef8e8d..ac81c90e0 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -634,7 +634,7 @@ async def run_update() -> None: return raise finally: - del self._in_progress_updates[job.id] + self._in_progress_updates.pop(job.id, None) self.create_task( run_update(), @@ -1722,7 +1722,7 @@ async def run_signal(): self._inbound.handle_signal(input) ) finally: - del self._in_progress_signals[id] + self._in_progress_signals.pop(id, None) self.create_task( run_signal(), diff --git a/temporalio/workflow.py b/temporalio/workflow.py index a976ea75f..2e735d26a 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -206,6 +206,27 @@ def signal( ... +@overload +def signal( + *, unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: + ... + + +@overload +def signal( + *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: + ... + + +@overload +def signal( + *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: + ... + + def signal( fn: Optional[CallableSyncOrAsyncReturnNoneType] = None, *, @@ -236,10 +257,19 @@ def signal( a running instance of this handler. """ - def with_name( - name: Optional[str], fn: CallableSyncOrAsyncReturnNoneType + def decorator( + name: Optional[str], + unfinished_handlers_policy: UnfinishedHandlersPolicy, + fn: CallableSyncOrAsyncReturnNoneType, ) -> CallableSyncOrAsyncReturnNoneType: - defn = _SignalDefinition(name=name, fn=fn, is_method=True) + if not name and not dynamic: + name = fn.__name__ + defn = _SignalDefinition( + name=name, + fn=fn, + is_method=True, + unfinished_handlers_policy=unfinished_handlers_policy, + ) setattr(fn, "__temporal_signal_definition", defn) if defn.dynamic_vararg: warnings.warn( @@ -249,13 +279,12 @@ def with_name( ) return fn - if name is not None or dynamic: + if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(with_name, name) - if fn is None: - raise RuntimeError("Cannot create signal without function or name or dynamic") - return with_name(fn.__name__, fn) + return partial(decorator, name, unfinished_handlers_policy) + else: + return decorator(fn.__name__, unfinished_handlers_policy, fn) @overload @@ -962,6 +991,36 @@ def update( ... +@overload +def update( + *, unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[ + [Callable[MultiParamSpec, ReturnType]], + UpdateMethodMultiParam[MultiParamSpec, ReturnType], +]: + ... + + +@overload +def update( + *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[ + [Callable[MultiParamSpec, ReturnType]], + UpdateMethodMultiParam[MultiParamSpec, ReturnType], +]: + ... + + +@overload +def update( + *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy +) -> Callable[ + [Callable[MultiParamSpec, ReturnType]], + UpdateMethodMultiParam[MultiParamSpec, ReturnType], +]: + ... + + def update( fn: Optional[CallableSyncOrAsyncType] = None, *, @@ -999,10 +1058,19 @@ def update( a running instance of this handler. """ - def with_name( - name: Optional[str], fn: CallableSyncOrAsyncType + def decorator( + name: Optional[str], + unfinished_handlers_policy: UnfinishedHandlersPolicy, + fn: CallableSyncOrAsyncType, ) -> CallableSyncOrAsyncType: - defn = _UpdateDefinition(name=name, fn=fn, is_method=True) + if not name and not dynamic: + name = fn.__name__ + defn = _UpdateDefinition( + name=name, + fn=fn, + is_method=True, + unfinished_handlers_policy=unfinished_handlers_policy, + ) if defn.dynamic_vararg: raise RuntimeError( "Dynamic updates do not support a vararg third param, use Sequence[RawValue]", @@ -1011,13 +1079,12 @@ def with_name( setattr(fn, "validator", partial(_update_validator, defn)) return fn - if name is not None or dynamic: + if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(with_name, name) - if fn is None: - raise RuntimeError("Cannot create update without function or name or dynamic") - return with_name(fn.__name__, fn) + return partial(decorator, name, unfinished_handlers_policy) + else: + return decorator(fn.__name__, unfinished_handlers_policy, fn) def _update_validator( diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 965a9dfbd..ab212e359 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -14,7 +14,6 @@ from dataclasses import dataclass from datetime import datetime, timedelta, timezone from enum import IntEnum -from functools import partial from typing import ( Any, Awaitable, @@ -5193,15 +5192,41 @@ async def run(self, wait_for_handlers: bool) -> None: self.handler_may_return = True await workflow.wait_condition(workflow.all_handlers_finished) - @workflow.update - async def my_update(self) -> None: + async def _do_update_or_signal(self) -> None: self.started_handler = True await workflow.wait_condition(lambda: self.handler_may_return) + @workflow.update + async def my_update(self) -> None: + await self._do_update_or_signal() + + @workflow.update( + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON + ) + async def my_update_ABANDON(self) -> None: + await self._do_update_or_signal() + + @workflow.update( + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + ) + async def my_update_WARN_AND_ABANDON(self) -> None: + await self._do_update_or_signal() + @workflow.signal async def my_signal(self): - self.started_handler = True - await workflow.wait_condition(lambda: self.handler_may_return) + await self._do_update_or_signal() + + @workflow.signal( + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON + ) + async def my_signal_ABANDON(self): + await self._do_update_or_signal() + + @workflow.signal( + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + ) + async def my_signal_WARN_AND_ABANDON(self): + await self._do_update_or_signal() async def test_unfinished_update_handlers(client: Client): @@ -5223,14 +5248,34 @@ async def run_test(self): assert any( self.is_unfinished_handler_warning(w) for w in await self.capture_warnings_from_workflow( - worker, wait_for_handlers=False + worker, + wait_for_handlers=False, ) ) - # but not when the workflow waits for handlers to complete. + # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, + assert any( + self.is_unfinished_handler_warning(w) + for w in await self.capture_warnings_from_workflow( + worker, + wait_for_handlers=False, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, + ) + ) + # but not when the workflow waits for handlers to complete, assert not any( self.is_unfinished_handler_warning(w) for w in await self.capture_warnings_from_workflow( - worker, wait_for_handlers=True + worker, + wait_for_handlers=True, + ) + ) + # nor when the silence-warnings policy is set on the handler. + assert not any( + self.is_unfinished_handler_warning(w) + for w in await self.capture_warnings_from_workflow( + worker, + wait_for_handlers=False, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, ) ) @@ -5238,6 +5283,7 @@ async def capture_warnings_from_workflow( self, worker: Worker, wait_for_handlers: bool, + unfinished_handlers_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, ) -> pytest.WarningsRecorder: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, @@ -5245,11 +5291,12 @@ async def capture_warnings_from_workflow( id=f"wf-{uuid.uuid4()}", task_queue=worker.task_queue, ) + handler_name = f"my_{self.handler_type}" + if unfinished_handlers_policy: + handler_name += f"_{unfinished_handlers_policy.name}" with pytest.WarningsRecorder() as warnings: if self.handler_type == "signal": - await asyncio.gather( - handle.signal(UnfinishedHandlersWorkflow.my_signal) - ) + await asyncio.gather(handle.signal(handler_name)) # In the case of signal, we must wait here for the workflow to complete to guarantee # that we'll capture the warning emitted by the worker. await handle.result() @@ -5257,9 +5304,7 @@ async def capture_warnings_from_workflow( if not wait_for_handlers: with pytest.raises(RPCError) as err: await asyncio.gather( - handle.execute_update( - UnfinishedHandlersWorkflow.my_update, id="update1" - ) + handle.execute_update(handler_name, id="my-update") ) assert ( err.value.status == RPCStatusCode.NOT_FOUND @@ -5268,9 +5313,7 @@ async def capture_warnings_from_workflow( ) else: await asyncio.gather( - handle.execute_update( - UnfinishedHandlersWorkflow.my_update, id="update1" - ) + handle.execute_update(handler_name, id="my-update") ) return warnings From 9c34eae3fe9b2cf85813d8149a44badedcef4fcf Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 13:24:05 -0400 Subject: [PATCH 11/45] Store (job, defn) --- temporalio/worker/_workflow_instance.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index ac81c90e0..6379793ea 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -275,14 +275,14 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: int, Tuple[ temporalio.bridge.proto.workflow_activation.SignalWorkflow, - temporalio.workflow.UnfinishedHandlersPolicy, + temporalio.workflow._SignalDefinition, ], ] = {} self._in_progress_updates: dict[ str, Tuple[ temporalio.bridge.proto.workflow_activation.DoUpdate, - temporalio.workflow.UnfinishedHandlersPolicy, + temporalio.workflow._UpdateDefinition, ], ] = {} @@ -460,13 +460,15 @@ def activate( if seen_completion: if any( - policy == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - for _, policy in self._in_progress_updates.values() + defn.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + for _, defn in self._in_progress_updates.values() ): warnings.warn(UNFINISHED_UPDATE_HANDLERS_WARNING, RuntimeWarning) if any( - policy == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - for _, policy in self._in_progress_signals.values() + defn.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + for _, defn in self._in_progress_signals.values() ): warnings.warn(UNFINISHED_SIGNAL_HANDLERS_WARNING, RuntimeWarning) return self._current_completion @@ -547,10 +549,7 @@ async def run_update() -> None: f"Update handler for '{job.name}' expected but not found, and there is no dynamic handler. " f"known updates: [{' '.join(known_updates)}]" ) - self._in_progress_updates[job.id] = ( - job, - defn.unfinished_handlers_policy, - ) + self._in_progress_updates[job.id] = (job, defn) args = self._process_handler_args( job.name, job.input, @@ -1717,7 +1716,7 @@ async def run_signal(): try: self._handled_signals_seq += 1 id = self._handled_signals_seq - self._in_progress_signals[id] = (job, defn.unfinished_handlers_policy) + self._in_progress_signals[id] = (job, defn) await self._run_top_level_workflow_function( self._inbound.handle_signal(input) ) From 6feed467c3c35c655b7c35e43cb5ff47444df094 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 19 Jun 2024 15:14:37 -0400 Subject: [PATCH 12/45] Format warning messages --- temporalio/worker/_workflow_instance.py | 56 +++++++++++++++++++------ 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 6379793ea..d43f0c60c 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -6,6 +6,7 @@ import collections import contextvars import inspect +import json import logging import random import sys @@ -459,18 +460,7 @@ def activate( i += 1 if seen_completion: - if any( - defn.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - for _, defn in self._in_progress_updates.values() - ): - warnings.warn(UNFINISHED_UPDATE_HANDLERS_WARNING, RuntimeWarning) - if any( - defn.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - for _, defn in self._in_progress_signals.values() - ): - warnings.warn(UNFINISHED_SIGNAL_HANDLERS_WARNING, RuntimeWarning) + self._warn_if_unfinished_handlers() return self._current_completion def _apply( @@ -1661,6 +1651,48 @@ def _is_workflow_failure_exception(self, err: BaseException) -> bool: ) ) + def _warn_if_unfinished_handlers(self) -> None: + updates_warning = self._make_unfinished_update_handlers_warning() + if updates_warning: + warnings.warn(updates_warning, RuntimeWarning) + + signals_warning = self._make_unfinished_signal_handlers_warning() + if signals_warning: + warnings.warn(signals_warning, RuntimeWarning) + + def _make_unfinished_update_handlers_warning(self) -> Optional[str]: + warnable_jobs = [ + job + for job, defn in self._in_progress_updates.values() + if defn.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + ] + if not warnable_jobs: + return None + handler_information = ( + "The following updates were unfinished (and warnings were not disabled for their handler): " + + json.dumps([{"name": j.name, "id": j.id} for j in warnable_jobs]) + ) + return UNFINISHED_UPDATE_HANDLERS_WARNING + " " + handler_information + + def _make_unfinished_signal_handlers_warning(self) -> Optional[str]: + warnable_jobs = [ + job + for job, defn in self._in_progress_signals.values() + if defn.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + ] + if not warnable_jobs: + return None + names = collections.Counter(j.signal_name for j in warnable_jobs) + handler_information = ( + "The following signals were unfinished (and warnings were not disabled for their handler): " + + json.dumps( + [{"name": name, "count": count} for name, count in names.most_common()] + ) + ) + return UNFINISHED_SIGNAL_HANDLERS_WARNING + " " + handler_information + def _next_seq(self, type: str) -> int: seq = self._curr_seqs.get(type, 0) + 1 self._curr_seqs[type] = seq From e024ef2ab7a4179f550f10a3fba4b5c64faffaf3 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 09:53:37 -0400 Subject: [PATCH 13/45] Improve test --- tests/worker/test_workflow.py | 87 ++++++++++++++++------------------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index ab212e359..a4f8525a3 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5184,17 +5184,20 @@ class UnfinishedHandlersWorkflow: def __init__(self): self.started_handler = False self.handler_may_return = False + self.handler_finished = False @workflow.run - async def run(self, wait_for_handlers: bool) -> None: + async def run(self, wait_for_handlers: bool) -> bool: await workflow.wait_condition(lambda: self.started_handler) if wait_for_handlers: self.handler_may_return = True await workflow.wait_condition(workflow.all_handlers_finished) + return self.handler_finished async def _do_update_or_signal(self) -> None: self.started_handler = True await workflow.wait_condition(lambda: self.handler_may_return) + self.handler_finished = True @workflow.update async def my_update(self) -> None: @@ -5230,66 +5233,55 @@ async def my_signal_WARN_AND_ABANDON(self): async def test_unfinished_update_handlers(client: Client): - await _UnfinishedHandlersTest(client, "update").run_test() + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + await _UnfinishedHandlersTest(client, worker, "update").run_test() async def test_unfinished_signal_handlers(client: Client): - await _UnfinishedHandlersTest(client, "signal").run_test() + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + await _UnfinishedHandlersTest(client, worker, "signal").run_test() @dataclass class _UnfinishedHandlersTest: client: Client + worker: Worker handler_type: Literal["update", "signal"] async def run_test(self): - async with new_worker(self.client, UnfinishedHandlersWorkflow) as worker: - # The unfinished handler warning is issued by default, - assert any( - self.is_unfinished_handler_warning(w) - for w in await self.capture_warnings_from_workflow( - worker, - wait_for_handlers=False, - ) - ) - # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, - assert any( - self.is_unfinished_handler_warning(w) - for w in await self.capture_warnings_from_workflow( - worker, - wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, - ) - ) - # but not when the workflow waits for handlers to complete, - assert not any( - self.is_unfinished_handler_warning(w) - for w in await self.capture_warnings_from_workflow( - worker, - wait_for_handlers=True, - ) - ) - # nor when the silence-warnings policy is set on the handler. - assert not any( - self.is_unfinished_handler_warning(w) - for w in await self.capture_warnings_from_workflow( - worker, - wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, - ) - ) - - async def capture_warnings_from_workflow( + # The unfinished handler warning is issued by default, + handler_finished, warning = await self.get_workflow_result_and_warning( + wait_for_handlers=False, + ) + assert not handler_finished and warning + # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, + handler_finished, warning = await self.get_workflow_result_and_warning( + wait_for_handlers=False, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, + ) + assert not handler_finished and warning + # but not when the workflow waits for handlers to complete, + handler_finished, warning = await self.get_workflow_result_and_warning( + wait_for_handlers=True, + ) + assert handler_finished and not warning + # nor when the silence-warnings policy is set on the handler. + handler_finished, warning = await self.get_workflow_result_and_warning( + wait_for_handlers=False, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, + ) + assert not handler_finished and not warning + + async def get_workflow_result_and_warning( self, - worker: Worker, wait_for_handlers: bool, unfinished_handlers_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, - ) -> pytest.WarningsRecorder: + ) -> Tuple[bool, bool]: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, arg=wait_for_handlers, id=f"wf-{uuid.uuid4()}", - task_queue=worker.task_queue, + task_queue=self.worker.task_queue, ) handler_name = f"my_{self.handler_type}" if unfinished_handlers_policy: @@ -5297,9 +5289,6 @@ async def capture_warnings_from_workflow( with pytest.WarningsRecorder() as warnings: if self.handler_type == "signal": await asyncio.gather(handle.signal(handler_name)) - # In the case of signal, we must wait here for the workflow to complete to guarantee - # that we'll capture the warning emitted by the worker. - await handle.result() else: if not wait_for_handlers: with pytest.raises(RPCError) as err: @@ -5316,7 +5305,11 @@ async def capture_warnings_from_workflow( handle.execute_update(handler_name, id="my-update") ) - return warnings + wf_result = await handle.result() + unfinished_handler_warning_emitted = any( + self.is_unfinished_handler_warning(w) for w in warnings + ) + return wf_result, unfinished_handler_warning_emitted def is_unfinished_handler_warning( self, From 2f96acdd0e94b3407493b58f0d7403f51d449cde Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 17:28:55 -0400 Subject: [PATCH 14/45] Clean up warning implementation --- temporalio/worker/_workflow_instance.py | 164 ++++++++++++------------ 1 file changed, 83 insertions(+), 81 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index d43f0c60c..e089ea921 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -27,6 +27,7 @@ Dict, Generator, Generic, + Iterable, Iterator, List, Mapping, @@ -79,31 +80,6 @@ # Set to true to log all cases where we're ignoring things during delete LOG_IGNORE_DURING_DELETE = False -UNFINISHED_UPDATE_HANDLERS_WARNING = """ -Workflow finished while update handlers are still running. This may have interrupted work that the -update handler was doing, and the client that sent the update will receive a 'workflow execution -already completed' RPCError instead of the update result. You can wait for all update and signal -handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. -Alternatively, if both you and the clients sending updates and signals are okay with interrupting -running handlers when the workflow finishes, and causing clients to receive errors, then you can -disable this warning via the update handler decorator: -`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. -""".replace( - "\n", " " -).strip() - -UNFINISHED_SIGNAL_HANDLERS_WARNING = """ -Workflow finished while signal handlers are still running. This may have interrupted work that the -signal handler was doing. You can wait for all update and signal handlers to complete by using -`await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both -you and the clients sending updates and signals are okay with interrupting running handlers when the -workflow finishes, and causing clients to receive errors, then you can disable this warning via the -signal handler decorator: -`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. -""".replace( - "\n", " " -).strip() - class WorkflowRunner(ABC): """Abstract runner for workflows that creates workflow instances to run. @@ -272,20 +248,8 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: # signals lack a unique per-invocation identifier, we introduce a sequence number for the # purpose. self._handled_signals_seq = 0 - self._in_progress_signals: dict[ - int, - Tuple[ - temporalio.bridge.proto.workflow_activation.SignalWorkflow, - temporalio.workflow._SignalDefinition, - ], - ] = {} - self._in_progress_updates: dict[ - str, - Tuple[ - temporalio.bridge.proto.workflow_activation.DoUpdate, - temporalio.workflow._UpdateDefinition, - ], - ] = {} + self._in_progress_signals: Dict[int, HandlerExecution] = {} + self._in_progress_updates: Dict[str, HandlerExecution] = {} # Add stack trace handler # TODO(cretz): Is it ok that this can be forcefully overridden by the @@ -539,7 +503,9 @@ async def run_update() -> None: f"Update handler for '{job.name}' expected but not found, and there is no dynamic handler. " f"known updates: [{' '.join(known_updates)}]" ) - self._in_progress_updates[job.id] = (job, defn) + self._in_progress_updates[job.id] = HandlerExecution( + job.name, defn.unfinished_handlers_policy, job.id + ) args = self._process_handler_args( job.name, job.input, @@ -1652,46 +1618,21 @@ def _is_workflow_failure_exception(self, err: BaseException) -> bool: ) def _warn_if_unfinished_handlers(self) -> None: - updates_warning = self._make_unfinished_update_handlers_warning() - if updates_warning: - warnings.warn(updates_warning, RuntimeWarning) - - signals_warning = self._make_unfinished_signal_handlers_warning() - if signals_warning: - warnings.warn(signals_warning, RuntimeWarning) - - def _make_unfinished_update_handlers_warning(self) -> Optional[str]: - warnable_jobs = [ - job - for job, defn in self._in_progress_updates.values() - if defn.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - ] - if not warnable_jobs: - return None - handler_information = ( - "The following updates were unfinished (and warnings were not disabled for their handler): " - + json.dumps([{"name": j.name, "id": j.id} for j in warnable_jobs]) - ) - return UNFINISHED_UPDATE_HANDLERS_WARNING + " " + handler_information - - def _make_unfinished_signal_handlers_warning(self) -> Optional[str]: - warnable_jobs = [ - job - for job, defn in self._in_progress_signals.values() - if defn.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON - ] - if not warnable_jobs: - return None - names = collections.Counter(j.signal_name for j in warnable_jobs) - handler_information = ( - "The following signals were unfinished (and warnings were not disabled for their handler): " - + json.dumps( - [{"name": name, "count": count} for name, count in names.most_common()] - ) - ) - return UNFINISHED_SIGNAL_HANDLERS_WARNING + " " + handler_information + def warnable(handler_executions: Iterable[HandlerExecution]): + return [ + ex + for ex in handler_executions + if ex.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + ] + + warnable_updates = warnable(self._in_progress_updates.values()) + if warnable_updates: + warnings.warn(UnfinishedUpdateHandlerWarning(warnable_updates)) + + warnable_signals = warnable(self._in_progress_signals.values()) + if warnable_signals: + warnings.warn(UnfinishedSignalHandlerWarning(warnable_signals)) def _next_seq(self, type: str) -> int: seq = self._curr_seqs.get(type, 0) + 1 @@ -1748,7 +1689,9 @@ async def run_signal(): try: self._handled_signals_seq += 1 id = self._handled_signals_seq - self._in_progress_signals[id] = (job, defn) + self._in_progress_signals[id] = HandlerExecution( + job.signal_name, defn.unfinished_handlers_policy + ) await self._run_top_level_workflow_function( self._inbound.handle_signal(input) ) @@ -2795,3 +2738,62 @@ def set( class _WorkflowBeingEvictedError(BaseException): pass + + +@dataclass +class HandlerExecution: + name: str + unfinished_handlers_policy: temporalio.workflow.UnfinishedHandlersPolicy + id: Optional[str] = None + + +class UnfinishedUpdateHandlerWarning(RuntimeWarning): + def __init__(self, handler_executions: List[HandlerExecution]) -> None: + super().__init__() + self.handler_executions = handler_executions + + def __str__(self) -> str: + message = """ +Workflow finished while update handlers are still running. This may have interrupted work that the +update handler was doing, and the client that sent the update will receive a 'workflow execution +already completed' RPCError instead of the update result. You can wait for all update and signal +handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. +Alternatively, if both you and the clients sending updates and signals are okay with interrupting +running handlers when the workflow finishes, and causing clients to receive errors, then you can +disable this warning via the update handler decorator: +`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +""".replace( + "\n", " " + ).strip() + return ( + f"{message} The following updates were unfinished (and warnings were not disabled for their handler): " + + json.dumps( + [{"name": ex.name, "id": ex.id} for ex in self.handler_executions] + ) + ) + + +class UnfinishedSignalHandlerWarning(RuntimeWarning): + def __init__(self, handler_executions: List[HandlerExecution]) -> None: + super().__init__() + self.handler_executions = handler_executions + + def __str__(self) -> str: + message = """ +Workflow finished while signal handlers are still running. This may have interrupted work that the +signal handler was doing. You can wait for all update and signal handlers to complete by using +`await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both +you and the clients sending updates and signals are okay with interrupting running handlers when the +workflow finishes, and causing clients to receive errors, then you can disable this warning via the +signal handler decorator: +`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +""".replace( + "\n", " " + ).strip() + names = collections.Counter(ex.name for ex in self.handler_executions) + return ( + f"{message} The following signals were unfinished (and warnings were not disabled for their handler): " + + json.dumps( + [{"name": name, "count": count} for name, count in names.most_common()] + ) + ) From 43387e47ffee3e65ac6bf105f2456cb5686e9b65 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 17:42:38 -0400 Subject: [PATCH 15/45] s/finished/complete/ --- temporalio/worker/_workflow_instance.py | 38 ++++++------- temporalio/workflow.py | 54 +++++++++---------- tests/worker/test_workflow.py | 72 ++++++++++++------------- 3 files changed, 82 insertions(+), 82 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index e089ea921..b126d43a0 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -244,7 +244,7 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: self._updates = dict(self._defn.updates) # We record in-progress signals and updates in order to support waiting for handlers to - # finish, and issuing warnings when the workflow exits with unfinished handlers. Since + # complete, and issuing warnings when the workflow exits with incomplete handlers. Since # signals lack a unique per-invocation identifier, we introduce a sequence number for the # purpose. self._handled_signals_seq = 0 @@ -424,7 +424,7 @@ def activate( i += 1 if seen_completion: - self._warn_if_unfinished_handlers() + self._warn_if_incomplete_handlers() return self._current_completion def _apply( @@ -504,7 +504,7 @@ async def run_update() -> None: f"known updates: [{' '.join(known_updates)}]" ) self._in_progress_updates[job.id] = HandlerExecution( - job.name, defn.unfinished_handlers_policy, job.id + job.name, defn.incomplete_handlers_policy, job.id ) args = self._process_handler_args( job.name, @@ -1120,7 +1120,7 @@ def workflow_set_update_handler( else: self._updates.pop(name, None) - def workflow_all_handlers_finished(self) -> bool: + def workflow_all_handlers_complete(self) -> bool: return not self._in_progress_updates and not self._in_progress_signals def workflow_start_activity( @@ -1617,22 +1617,22 @@ def _is_workflow_failure_exception(self, err: BaseException) -> bool: ) ) - def _warn_if_unfinished_handlers(self) -> None: + def _warn_if_incomplete_handlers(self) -> None: def warnable(handler_executions: Iterable[HandlerExecution]): return [ ex for ex in handler_executions - if ex.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + if ex.incomplete_handlers_policy + == temporalio.workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON ] warnable_updates = warnable(self._in_progress_updates.values()) if warnable_updates: - warnings.warn(UnfinishedUpdateHandlerWarning(warnable_updates)) + warnings.warn(IncompleteUpdateHandlerWarning(warnable_updates)) warnable_signals = warnable(self._in_progress_signals.values()) if warnable_signals: - warnings.warn(UnfinishedSignalHandlerWarning(warnable_signals)) + warnings.warn(IncompleteSignalHandlerWarning(warnable_signals)) def _next_seq(self, type: str) -> int: seq = self._curr_seqs.get(type, 0) + 1 @@ -1690,7 +1690,7 @@ async def run_signal(): self._handled_signals_seq += 1 id = self._handled_signals_seq self._in_progress_signals[id] = HandlerExecution( - job.signal_name, defn.unfinished_handlers_policy + job.signal_name, defn.incomplete_handlers_policy ) await self._run_top_level_workflow_function( self._inbound.handle_signal(input) @@ -2743,11 +2743,11 @@ class _WorkflowBeingEvictedError(BaseException): @dataclass class HandlerExecution: name: str - unfinished_handlers_policy: temporalio.workflow.UnfinishedHandlersPolicy + incomplete_handlers_policy: temporalio.workflow.IncompleteHandlersPolicy id: Optional[str] = None -class UnfinishedUpdateHandlerWarning(RuntimeWarning): +class IncompleteUpdateHandlerWarning(RuntimeWarning): def __init__(self, handler_executions: List[HandlerExecution]) -> None: super().__init__() self.handler_executions = handler_executions @@ -2757,23 +2757,23 @@ def __str__(self) -> str: Workflow finished while update handlers are still running. This may have interrupted work that the update handler was doing, and the client that sent the update will receive a 'workflow execution already completed' RPCError instead of the update result. You can wait for all update and signal -handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. +handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_complete())`. Alternatively, if both you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the update handler decorator: -`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +`@workflow.update(incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON)`. """.replace( "\n", " " ).strip() return ( - f"{message} The following updates were unfinished (and warnings were not disabled for their handler): " + f"{message} The following updates were incomplete (and warnings were not disabled for their handler): " + json.dumps( [{"name": ex.name, "id": ex.id} for ex in self.handler_executions] ) ) -class UnfinishedSignalHandlerWarning(RuntimeWarning): +class IncompleteSignalHandlerWarning(RuntimeWarning): def __init__(self, handler_executions: List[HandlerExecution]) -> None: super().__init__() self.handler_executions = handler_executions @@ -2782,17 +2782,17 @@ def __str__(self) -> str: message = """ Workflow finished while signal handlers are still running. This may have interrupted work that the signal handler was doing. You can wait for all update and signal handlers to complete by using -`await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both +`await workflow.wait_condition(lambda: workflow.all_handlers_complete())`. Alternatively, if both you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the signal handler decorator: -`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +`@workflow.signal(incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON)`. """.replace( "\n", " " ).strip() names = collections.Counter(ex.name for ex in self.handler_executions) return ( - f"{message} The following signals were unfinished (and warnings were not disabled for their handler): " + f"{message} The following signals were incomplete (and warnings were not disabled for their handler): " + json.dumps( [{"name": name, "count": count} for name, count in names.most_common()] ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 2e735d26a..7de6e5149 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -173,7 +173,7 @@ def run(fn: CallableAsyncType) -> CallableAsyncType: return fn # type: ignore[return-value] -class UnfinishedHandlersPolicy(IntEnum): +class IncompleteHandlersPolicy(IntEnum): """Actions taken if a workflow terminates with running handlers. Policy defining actions taken when a workflow exits while update or signal handlers are running. @@ -208,21 +208,21 @@ def signal( @overload def signal( - *, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, name: str, incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy + *, dynamic: Literal[True], incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -232,7 +232,7 @@ def signal( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + incomplete_handlers_policy: IncompleteHandlersPolicy = IncompleteHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow signal method. @@ -253,13 +253,13 @@ def signal( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - unfinished_handlers_policy: Actions taken if a workflow terminates with + incomplete_handlers_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - unfinished_handlers_policy: UnfinishedHandlersPolicy, + incomplete_handlers_policy: IncompleteHandlersPolicy, fn: CallableSyncOrAsyncReturnNoneType, ) -> CallableSyncOrAsyncReturnNoneType: if not name and not dynamic: @@ -268,7 +268,7 @@ def decorator( name=name, fn=fn, is_method=True, - unfinished_handlers_policy=unfinished_handlers_policy, + incomplete_handlers_policy=incomplete_handlers_policy, ) setattr(fn, "__temporal_signal_definition", defn) if defn.dynamic_vararg: @@ -282,9 +282,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, unfinished_handlers_policy) + return partial(decorator, name, incomplete_handlers_policy) else: - return decorator(fn.__name__, unfinished_handlers_policy, fn) + return decorator(fn.__name__, incomplete_handlers_policy, fn) @overload @@ -631,7 +631,7 @@ def workflow_set_update_handler( ... @abstractmethod - def workflow_all_handlers_finished(self) -> bool: + def workflow_all_handlers_complete(self) -> bool: ... @abstractmethod @@ -993,7 +993,7 @@ def update( @overload def update( - *, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1003,7 +1003,7 @@ def update( @overload def update( - *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, name: str, incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1013,7 +1013,7 @@ def update( @overload def update( - *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy + *, dynamic: Literal[True], incomplete_handlers_policy: IncompleteHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1026,7 +1026,7 @@ def update( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + incomplete_handlers_policy: IncompleteHandlersPolicy = IncompleteHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow update handler method. @@ -1054,13 +1054,13 @@ def update( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - unfinished_handlers_policy: Actions taken if a workflow terminates with + incomplete_handlers_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - unfinished_handlers_policy: UnfinishedHandlersPolicy, + incomplete_handlers_policy: IncompleteHandlersPolicy, fn: CallableSyncOrAsyncType, ) -> CallableSyncOrAsyncType: if not name and not dynamic: @@ -1069,7 +1069,7 @@ def decorator( name=name, fn=fn, is_method=True, - unfinished_handlers_policy=unfinished_handlers_policy, + incomplete_handlers_policy=incomplete_handlers_policy, ) if defn.dynamic_vararg: raise RuntimeError( @@ -1082,9 +1082,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, unfinished_handlers_policy) + return partial(decorator, name, incomplete_handlers_policy) else: - return decorator(fn.__name__, unfinished_handlers_policy, fn) + return decorator(fn.__name__, incomplete_handlers_policy, fn) def _update_validator( @@ -1541,8 +1541,8 @@ class _SignalDefinition: name: Optional[str] fn: Callable[..., Union[None, Awaitable[None]]] is_method: bool - unfinished_handlers_policy: UnfinishedHandlersPolicy = ( - UnfinishedHandlersPolicy.WARN_AND_ABANDON + incomplete_handlers_policy: IncompleteHandlersPolicy = ( + IncompleteHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None @@ -1625,8 +1625,8 @@ class _UpdateDefinition: name: Optional[str] fn: Callable[..., Union[Any, Awaitable[Any]]] is_method: bool - unfinished_handlers_policy: UnfinishedHandlersPolicy = ( - UnfinishedHandlersPolicy.WARN_AND_ABANDON + incomplete_handlers_policy: IncompleteHandlersPolicy = ( + IncompleteHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None @@ -4497,17 +4497,17 @@ def set_dynamic_update_handler( _Runtime.current().workflow_set_update_handler(None, handler, validator) -def all_handlers_finished() -> bool: +def all_handlers_complete() -> bool: """Whether update and signal handlers have finished executing. Consider waiting on this condition before workflow return or continue-as-new, to prevent interruption of in-progress handlers by workflow exit: - ``await workflow.wait_condition(lambda: workflow.all_handlers_finished())`` + ``await workflow.wait_condition(lambda: workflow.all_handlers_complete())`` Returns: True if there are no in-progress update or signal handler executions. """ - return _Runtime.current().workflow_all_handlers_finished() + return _Runtime.current().workflow_all_handlers_complete() def as_completed( diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index a4f8525a3..5e25d75c3 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5180,37 +5180,37 @@ async def test_workflow_current_update(client: Client, env: WorkflowEnvironment) @workflow.defn -class UnfinishedHandlersWorkflow: +class IncompleteHandlersWorkflow: def __init__(self): self.started_handler = False self.handler_may_return = False - self.handler_finished = False + self.handler_complete = False @workflow.run async def run(self, wait_for_handlers: bool) -> bool: await workflow.wait_condition(lambda: self.started_handler) if wait_for_handlers: self.handler_may_return = True - await workflow.wait_condition(workflow.all_handlers_finished) - return self.handler_finished + await workflow.wait_condition(workflow.all_handlers_complete) + return self.handler_complete async def _do_update_or_signal(self) -> None: self.started_handler = True await workflow.wait_condition(lambda: self.handler_may_return) - self.handler_finished = True + self.handler_complete = True @workflow.update async def my_update(self) -> None: await self._do_update_or_signal() @workflow.update( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON ) async def my_update_ABANDON(self) -> None: await self._do_update_or_signal() @workflow.update( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON ) async def my_update_WARN_AND_ABANDON(self) -> None: await self._do_update_or_signal() @@ -5220,72 +5220,72 @@ async def my_signal(self): await self._do_update_or_signal() @workflow.signal( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON ) async def my_signal_ABANDON(self): await self._do_update_or_signal() @workflow.signal( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON ) async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() -async def test_unfinished_update_handlers(client: Client): - async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - await _UnfinishedHandlersTest(client, worker, "update").run_test() +async def test_incomplete_update_handlers(client: Client): + async with new_worker(client, IncompleteHandlersWorkflow) as worker: + await _IncompleteHandlersTest(client, worker, "update").run_test() -async def test_unfinished_signal_handlers(client: Client): - async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - await _UnfinishedHandlersTest(client, worker, "signal").run_test() +async def test_incomplete_signal_handlers(client: Client): + async with new_worker(client, IncompleteHandlersWorkflow) as worker: + await _IncompleteHandlersTest(client, worker, "signal").run_test() @dataclass -class _UnfinishedHandlersTest: +class _IncompleteHandlersTest: client: Client worker: Worker handler_type: Literal["update", "signal"] async def run_test(self): - # The unfinished handler warning is issued by default, - handler_finished, warning = await self.get_workflow_result_and_warning( + # The incomplete handler warning is issued by default, + handler_complete, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, ) - assert not handler_finished and warning - # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, - handler_finished, warning = await self.get_workflow_result_and_warning( + assert not handler_complete and warning + # and when the workflow sets the incomplete_handlers_policy to WARN_AND_ABANDON, + handler_complete, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON, ) - assert not handler_finished and warning + assert not handler_complete and warning # but not when the workflow waits for handlers to complete, - handler_finished, warning = await self.get_workflow_result_and_warning( + handler_complete, warning = await self.get_workflow_result_and_warning( wait_for_handlers=True, ) - assert handler_finished and not warning + assert handler_complete and not warning # nor when the silence-warnings policy is set on the handler. - handler_finished, warning = await self.get_workflow_result_and_warning( + handler_complete, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, + incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON, ) - assert not handler_finished and not warning + assert not handler_complete and not warning async def get_workflow_result_and_warning( self, wait_for_handlers: bool, - unfinished_handlers_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, + incomplete_handlers_policy: Optional[workflow.IncompleteHandlersPolicy] = None, ) -> Tuple[bool, bool]: handle = await self.client.start_workflow( - UnfinishedHandlersWorkflow.run, + IncompleteHandlersWorkflow.run, arg=wait_for_handlers, id=f"wf-{uuid.uuid4()}", task_queue=self.worker.task_queue, ) handler_name = f"my_{self.handler_type}" - if unfinished_handlers_policy: - handler_name += f"_{unfinished_handlers_policy.name}" + if incomplete_handlers_policy: + handler_name += f"_{incomplete_handlers_policy.name}" with pytest.WarningsRecorder() as warnings: if self.handler_type == "signal": await asyncio.gather(handle.signal(handler_name)) @@ -5306,12 +5306,12 @@ async def get_workflow_result_and_warning( ) wf_result = await handle.result() - unfinished_handler_warning_emitted = any( - self.is_unfinished_handler_warning(w) for w in warnings + incomplete_handler_warning_emitted = any( + self.is_incomplete_handler_warning(w) for w in warnings ) - return wf_result, unfinished_handler_warning_emitted + return wf_result, incomplete_handler_warning_emitted - def is_unfinished_handler_warning( + def is_incomplete_handler_warning( self, warning: warnings.WarningMessage, ) -> bool: From 359ce602198846e30bfd41ddab45197d75ca0020 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 17:53:57 -0400 Subject: [PATCH 16/45] Revert "s/finished/complete/" This reverts commit 6221f909ae653f30b2b5baa109328276d79166a1. --- temporalio/worker/_workflow_instance.py | 38 ++++++------- temporalio/workflow.py | 54 +++++++++---------- tests/worker/test_workflow.py | 72 ++++++++++++------------- 3 files changed, 82 insertions(+), 82 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index b126d43a0..e089ea921 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -244,7 +244,7 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: self._updates = dict(self._defn.updates) # We record in-progress signals and updates in order to support waiting for handlers to - # complete, and issuing warnings when the workflow exits with incomplete handlers. Since + # finish, and issuing warnings when the workflow exits with unfinished handlers. Since # signals lack a unique per-invocation identifier, we introduce a sequence number for the # purpose. self._handled_signals_seq = 0 @@ -424,7 +424,7 @@ def activate( i += 1 if seen_completion: - self._warn_if_incomplete_handlers() + self._warn_if_unfinished_handlers() return self._current_completion def _apply( @@ -504,7 +504,7 @@ async def run_update() -> None: f"known updates: [{' '.join(known_updates)}]" ) self._in_progress_updates[job.id] = HandlerExecution( - job.name, defn.incomplete_handlers_policy, job.id + job.name, defn.unfinished_handlers_policy, job.id ) args = self._process_handler_args( job.name, @@ -1120,7 +1120,7 @@ def workflow_set_update_handler( else: self._updates.pop(name, None) - def workflow_all_handlers_complete(self) -> bool: + def workflow_all_handlers_finished(self) -> bool: return not self._in_progress_updates and not self._in_progress_signals def workflow_start_activity( @@ -1617,22 +1617,22 @@ def _is_workflow_failure_exception(self, err: BaseException) -> bool: ) ) - def _warn_if_incomplete_handlers(self) -> None: + def _warn_if_unfinished_handlers(self) -> None: def warnable(handler_executions: Iterable[HandlerExecution]): return [ ex for ex in handler_executions - if ex.incomplete_handlers_policy - == temporalio.workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON + if ex.unfinished_handlers_policy + == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON ] warnable_updates = warnable(self._in_progress_updates.values()) if warnable_updates: - warnings.warn(IncompleteUpdateHandlerWarning(warnable_updates)) + warnings.warn(UnfinishedUpdateHandlerWarning(warnable_updates)) warnable_signals = warnable(self._in_progress_signals.values()) if warnable_signals: - warnings.warn(IncompleteSignalHandlerWarning(warnable_signals)) + warnings.warn(UnfinishedSignalHandlerWarning(warnable_signals)) def _next_seq(self, type: str) -> int: seq = self._curr_seqs.get(type, 0) + 1 @@ -1690,7 +1690,7 @@ async def run_signal(): self._handled_signals_seq += 1 id = self._handled_signals_seq self._in_progress_signals[id] = HandlerExecution( - job.signal_name, defn.incomplete_handlers_policy + job.signal_name, defn.unfinished_handlers_policy ) await self._run_top_level_workflow_function( self._inbound.handle_signal(input) @@ -2743,11 +2743,11 @@ class _WorkflowBeingEvictedError(BaseException): @dataclass class HandlerExecution: name: str - incomplete_handlers_policy: temporalio.workflow.IncompleteHandlersPolicy + unfinished_handlers_policy: temporalio.workflow.UnfinishedHandlersPolicy id: Optional[str] = None -class IncompleteUpdateHandlerWarning(RuntimeWarning): +class UnfinishedUpdateHandlerWarning(RuntimeWarning): def __init__(self, handler_executions: List[HandlerExecution]) -> None: super().__init__() self.handler_executions = handler_executions @@ -2757,23 +2757,23 @@ def __str__(self) -> str: Workflow finished while update handlers are still running. This may have interrupted work that the update handler was doing, and the client that sent the update will receive a 'workflow execution already completed' RPCError instead of the update result. You can wait for all update and signal -handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_complete())`. +handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the update handler decorator: -`@workflow.update(incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON)`. +`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. """.replace( "\n", " " ).strip() return ( - f"{message} The following updates were incomplete (and warnings were not disabled for their handler): " + f"{message} The following updates were unfinished (and warnings were not disabled for their handler): " + json.dumps( [{"name": ex.name, "id": ex.id} for ex in self.handler_executions] ) ) -class IncompleteSignalHandlerWarning(RuntimeWarning): +class UnfinishedSignalHandlerWarning(RuntimeWarning): def __init__(self, handler_executions: List[HandlerExecution]) -> None: super().__init__() self.handler_executions = handler_executions @@ -2782,17 +2782,17 @@ def __str__(self) -> str: message = """ Workflow finished while signal handlers are still running. This may have interrupted work that the signal handler was doing. You can wait for all update and signal handlers to complete by using -`await workflow.wait_condition(lambda: workflow.all_handlers_complete())`. Alternatively, if both +`await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the signal handler decorator: -`@workflow.signal(incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON)`. +`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. """.replace( "\n", " " ).strip() names = collections.Counter(ex.name for ex in self.handler_executions) return ( - f"{message} The following signals were incomplete (and warnings were not disabled for their handler): " + f"{message} The following signals were unfinished (and warnings were not disabled for their handler): " + json.dumps( [{"name": name, "count": count} for name, count in names.most_common()] ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 7de6e5149..2e735d26a 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -173,7 +173,7 @@ def run(fn: CallableAsyncType) -> CallableAsyncType: return fn # type: ignore[return-value] -class IncompleteHandlersPolicy(IntEnum): +class UnfinishedHandlersPolicy(IntEnum): """Actions taken if a workflow terminates with running handlers. Policy defining actions taken when a workflow exits while update or signal handlers are running. @@ -208,21 +208,21 @@ def signal( @overload def signal( - *, incomplete_handlers_policy: IncompleteHandlersPolicy + *, unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, name: str, incomplete_handlers_policy: IncompleteHandlersPolicy + *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, dynamic: Literal[True], incomplete_handlers_policy: IncompleteHandlersPolicy + *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -232,7 +232,7 @@ def signal( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - incomplete_handlers_policy: IncompleteHandlersPolicy = IncompleteHandlersPolicy.WARN_AND_ABANDON, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow signal method. @@ -253,13 +253,13 @@ def signal( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - incomplete_handlers_policy: Actions taken if a workflow terminates with + unfinished_handlers_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - incomplete_handlers_policy: IncompleteHandlersPolicy, + unfinished_handlers_policy: UnfinishedHandlersPolicy, fn: CallableSyncOrAsyncReturnNoneType, ) -> CallableSyncOrAsyncReturnNoneType: if not name and not dynamic: @@ -268,7 +268,7 @@ def decorator( name=name, fn=fn, is_method=True, - incomplete_handlers_policy=incomplete_handlers_policy, + unfinished_handlers_policy=unfinished_handlers_policy, ) setattr(fn, "__temporal_signal_definition", defn) if defn.dynamic_vararg: @@ -282,9 +282,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, incomplete_handlers_policy) + return partial(decorator, name, unfinished_handlers_policy) else: - return decorator(fn.__name__, incomplete_handlers_policy, fn) + return decorator(fn.__name__, unfinished_handlers_policy, fn) @overload @@ -631,7 +631,7 @@ def workflow_set_update_handler( ... @abstractmethod - def workflow_all_handlers_complete(self) -> bool: + def workflow_all_handlers_finished(self) -> bool: ... @abstractmethod @@ -993,7 +993,7 @@ def update( @overload def update( - *, incomplete_handlers_policy: IncompleteHandlersPolicy + *, unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1003,7 +1003,7 @@ def update( @overload def update( - *, name: str, incomplete_handlers_policy: IncompleteHandlersPolicy + *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1013,7 +1013,7 @@ def update( @overload def update( - *, dynamic: Literal[True], incomplete_handlers_policy: IncompleteHandlersPolicy + *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1026,7 +1026,7 @@ def update( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - incomplete_handlers_policy: IncompleteHandlersPolicy = IncompleteHandlersPolicy.WARN_AND_ABANDON, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow update handler method. @@ -1054,13 +1054,13 @@ def update( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - incomplete_handlers_policy: Actions taken if a workflow terminates with + unfinished_handlers_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - incomplete_handlers_policy: IncompleteHandlersPolicy, + unfinished_handlers_policy: UnfinishedHandlersPolicy, fn: CallableSyncOrAsyncType, ) -> CallableSyncOrAsyncType: if not name and not dynamic: @@ -1069,7 +1069,7 @@ def decorator( name=name, fn=fn, is_method=True, - incomplete_handlers_policy=incomplete_handlers_policy, + unfinished_handlers_policy=unfinished_handlers_policy, ) if defn.dynamic_vararg: raise RuntimeError( @@ -1082,9 +1082,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, incomplete_handlers_policy) + return partial(decorator, name, unfinished_handlers_policy) else: - return decorator(fn.__name__, incomplete_handlers_policy, fn) + return decorator(fn.__name__, unfinished_handlers_policy, fn) def _update_validator( @@ -1541,8 +1541,8 @@ class _SignalDefinition: name: Optional[str] fn: Callable[..., Union[None, Awaitable[None]]] is_method: bool - incomplete_handlers_policy: IncompleteHandlersPolicy = ( - IncompleteHandlersPolicy.WARN_AND_ABANDON + unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + UnfinishedHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None @@ -1625,8 +1625,8 @@ class _UpdateDefinition: name: Optional[str] fn: Callable[..., Union[Any, Awaitable[Any]]] is_method: bool - incomplete_handlers_policy: IncompleteHandlersPolicy = ( - IncompleteHandlersPolicy.WARN_AND_ABANDON + unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + UnfinishedHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None @@ -4497,17 +4497,17 @@ def set_dynamic_update_handler( _Runtime.current().workflow_set_update_handler(None, handler, validator) -def all_handlers_complete() -> bool: +def all_handlers_finished() -> bool: """Whether update and signal handlers have finished executing. Consider waiting on this condition before workflow return or continue-as-new, to prevent interruption of in-progress handlers by workflow exit: - ``await workflow.wait_condition(lambda: workflow.all_handlers_complete())`` + ``await workflow.wait_condition(lambda: workflow.all_handlers_finished())`` Returns: True if there are no in-progress update or signal handler executions. """ - return _Runtime.current().workflow_all_handlers_complete() + return _Runtime.current().workflow_all_handlers_finished() def as_completed( diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 5e25d75c3..a4f8525a3 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5180,37 +5180,37 @@ async def test_workflow_current_update(client: Client, env: WorkflowEnvironment) @workflow.defn -class IncompleteHandlersWorkflow: +class UnfinishedHandlersWorkflow: def __init__(self): self.started_handler = False self.handler_may_return = False - self.handler_complete = False + self.handler_finished = False @workflow.run async def run(self, wait_for_handlers: bool) -> bool: await workflow.wait_condition(lambda: self.started_handler) if wait_for_handlers: self.handler_may_return = True - await workflow.wait_condition(workflow.all_handlers_complete) - return self.handler_complete + await workflow.wait_condition(workflow.all_handlers_finished) + return self.handler_finished async def _do_update_or_signal(self) -> None: self.started_handler = True await workflow.wait_condition(lambda: self.handler_may_return) - self.handler_complete = True + self.handler_finished = True @workflow.update async def my_update(self) -> None: await self._do_update_or_signal() @workflow.update( - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON ) async def my_update_ABANDON(self) -> None: await self._do_update_or_signal() @workflow.update( - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON ) async def my_update_WARN_AND_ABANDON(self) -> None: await self._do_update_or_signal() @@ -5220,72 +5220,72 @@ async def my_signal(self): await self._do_update_or_signal() @workflow.signal( - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON ) async def my_signal_ABANDON(self): await self._do_update_or_signal() @workflow.signal( - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON ) async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() -async def test_incomplete_update_handlers(client: Client): - async with new_worker(client, IncompleteHandlersWorkflow) as worker: - await _IncompleteHandlersTest(client, worker, "update").run_test() +async def test_unfinished_update_handlers(client: Client): + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + await _UnfinishedHandlersTest(client, worker, "update").run_test() -async def test_incomplete_signal_handlers(client: Client): - async with new_worker(client, IncompleteHandlersWorkflow) as worker: - await _IncompleteHandlersTest(client, worker, "signal").run_test() +async def test_unfinished_signal_handlers(client: Client): + async with new_worker(client, UnfinishedHandlersWorkflow) as worker: + await _UnfinishedHandlersTest(client, worker, "signal").run_test() @dataclass -class _IncompleteHandlersTest: +class _UnfinishedHandlersTest: client: Client worker: Worker handler_type: Literal["update", "signal"] async def run_test(self): - # The incomplete handler warning is issued by default, - handler_complete, warning = await self.get_workflow_result_and_warning( + # The unfinished handler warning is issued by default, + handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, ) - assert not handler_complete and warning - # and when the workflow sets the incomplete_handlers_policy to WARN_AND_ABANDON, - handler_complete, warning = await self.get_workflow_result_and_warning( + assert not handler_finished and warning + # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, + handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.WARN_AND_ABANDON, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) - assert not handler_complete and warning + assert not handler_finished and warning # but not when the workflow waits for handlers to complete, - handler_complete, warning = await self.get_workflow_result_and_warning( + handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=True, ) - assert handler_complete and not warning + assert handler_finished and not warning # nor when the silence-warnings policy is set on the handler. - handler_complete, warning = await self.get_workflow_result_and_warning( + handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - incomplete_handlers_policy=workflow.IncompleteHandlersPolicy.ABANDON, + unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, ) - assert not handler_complete and not warning + assert not handler_finished and not warning async def get_workflow_result_and_warning( self, wait_for_handlers: bool, - incomplete_handlers_policy: Optional[workflow.IncompleteHandlersPolicy] = None, + unfinished_handlers_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, ) -> Tuple[bool, bool]: handle = await self.client.start_workflow( - IncompleteHandlersWorkflow.run, + UnfinishedHandlersWorkflow.run, arg=wait_for_handlers, id=f"wf-{uuid.uuid4()}", task_queue=self.worker.task_queue, ) handler_name = f"my_{self.handler_type}" - if incomplete_handlers_policy: - handler_name += f"_{incomplete_handlers_policy.name}" + if unfinished_handlers_policy: + handler_name += f"_{unfinished_handlers_policy.name}" with pytest.WarningsRecorder() as warnings: if self.handler_type == "signal": await asyncio.gather(handle.signal(handler_name)) @@ -5306,12 +5306,12 @@ async def get_workflow_result_and_warning( ) wf_result = await handle.result() - incomplete_handler_warning_emitted = any( - self.is_incomplete_handler_warning(w) for w in warnings + unfinished_handler_warning_emitted = any( + self.is_unfinished_handler_warning(w) for w in warnings ) - return wf_result, incomplete_handler_warning_emitted + return wf_result, unfinished_handler_warning_emitted - def is_incomplete_handler_warning( + def is_unfinished_handler_warning( self, warning: warnings.WarningMessage, ) -> bool: From bb2bf5c66f2fb4537a429b64281df8914f2ee70c Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 18:17:33 -0400 Subject: [PATCH 17/45] Wrap signal handling without introducing new yield point --- temporalio/worker/_workflow_instance.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index e089ea921..dc8dfa6d4 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -1685,23 +1685,20 @@ def _process_signal_job( signal=job.signal_name, args=args, headers=job.headers ) - async def run_signal(): - try: - self._handled_signals_seq += 1 - id = self._handled_signals_seq - self._in_progress_signals[id] = HandlerExecution( - job.signal_name, defn.unfinished_handlers_policy - ) - await self._run_top_level_workflow_function( - self._inbound.handle_signal(input) - ) - finally: - self._in_progress_signals.pop(id, None) + self._handled_signals_seq += 1 + id = self._handled_signals_seq + self._in_progress_signals[id] = HandlerExecution( + job.signal_name, defn.unfinished_handlers_policy + ) - self.create_task( - run_signal(), + def done_callback(f): + self._in_progress_signals.pop(id, None) + + task = self.create_task( + self._run_top_level_workflow_function(self._inbound.handle_signal(input)), name=f"signal: {job.signal_name}", ) + task.add_done_callback(done_callback) def _register_task( self, From fd1a2699f3083e9ebd8d7bde068b8dc44b780623 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 18:22:02 -0400 Subject: [PATCH 18/45] List default value first in enum --- temporalio/workflow.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 2e735d26a..313b5005a 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -180,11 +180,11 @@ class UnfinishedHandlersPolicy(IntEnum): The workflow exit may be due to successful return, failure, cancellation, or continue-as-new. """ + # Issue a warning in addition to abandoning. + WARN_AND_ABANDON = 1 # Abandon the handler. In the case of an update handler this means that the client will receive # an error rather than the update result. - ABANDON = 1 - # Issue a warning in addition to abandoning. - WARN_AND_ABANDON = 2 + ABANDON = 2 @overload From e218fe27e25a281166a988e8a33dcd34dadedbfa Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 18:23:04 -0400 Subject: [PATCH 19/45] s/IntEnum/Enum/ --- temporalio/workflow.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 313b5005a..94ad9d7d2 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -173,7 +173,7 @@ def run(fn: CallableAsyncType) -> CallableAsyncType: return fn # type: ignore[return-value] -class UnfinishedHandlersPolicy(IntEnum): +class UnfinishedHandlersPolicy(Enum): """Actions taken if a workflow terminates with running handlers. Policy defining actions taken when a workflow exits while update or signal handlers are running. From 866b9be50d509a3df416cc2e7373770cc80161cc Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 20 Jun 2024 18:31:40 -0400 Subject: [PATCH 20/45] Maintain alphabetical method order --- temporalio/worker/_workflow_instance.py | 6 +++--- temporalio/workflow.py | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index dc8dfa6d4..bb5544c3e 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -887,6 +887,9 @@ def _apply_update_random_seed( #### _Runtime direct workflow call overrides #### # These are in alphabetical order and all start with "workflow_". + def workflow_all_handlers_finished(self) -> bool: + return not self._in_progress_updates and not self._in_progress_signals + def workflow_continue_as_new( self, *args: Any, @@ -1120,9 +1123,6 @@ def workflow_set_update_handler( else: self._updates.pop(name, None) - def workflow_all_handlers_finished(self) -> bool: - return not self._in_progress_updates and not self._in_progress_signals - def workflow_start_activity( self, activity: Any, diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 94ad9d7d2..e2214e02b 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -513,6 +513,10 @@ def logger_details(self) -> Mapping[str, Any]: self._logger_details = self.workflow_info()._logger_details() return self._logger_details + @abstractmethod + def workflow_all_handlers_finished(self) -> bool: + ... + @abstractmethod def workflow_continue_as_new( self, @@ -630,10 +634,6 @@ def workflow_set_update_handler( ) -> None: ... - @abstractmethod - def workflow_all_handlers_finished(self) -> bool: - ... - @abstractmethod def workflow_start_activity( self, From 7d13e1d1e29f788d8d50c17b76de766f84ac8d01 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 11:28:13 -0400 Subject: [PATCH 21/45] Simplify overloads --- temporalio/workflow.py | 56 ++++++++++++------------------------------ 1 file changed, 16 insertions(+), 40 deletions(-) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index e2214e02b..0e2cdb22f 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -194,35 +194,26 @@ def signal(fn: CallableSyncOrAsyncReturnNoneType) -> CallableSyncOrAsyncReturnNo @overload def signal( - *, name: str -) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: - ... - - -@overload -def signal( - *, dynamic: Literal[True] -) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: - ... - - -@overload -def signal( - *, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + name: str, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @overload def signal( - *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + dynamic: Literal[True], + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -973,27 +964,8 @@ def update( @overload def update( - *, name: str -) -> Callable[ - [Callable[MultiParamSpec, ReturnType]], - UpdateMethodMultiParam[MultiParamSpec, ReturnType], -]: - ... - - -@overload -def update( - *, dynamic: Literal[True] -) -> Callable[ - [Callable[MultiParamSpec, ReturnType]], - UpdateMethodMultiParam[MultiParamSpec, ReturnType], -]: - ... - - -@overload -def update( - *, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1003,7 +975,9 @@ def update( @overload def update( - *, name: str, unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + name: str, + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1013,7 +987,9 @@ def update( @overload def update( - *, dynamic: Literal[True], unfinished_handlers_policy: UnfinishedHandlersPolicy + *, + dynamic: Literal[True], + unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], From ec318ae47ab165cc11b7a836a1fb808685c7f145 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 11:38:13 -0400 Subject: [PATCH 22/45] Abbreviate decorator argument --- temporalio/worker/_workflow_instance.py | 4 +-- temporalio/workflow.py | 40 ++++++++++++------------- tests/worker/test_workflow.py | 22 ++++++-------- 3 files changed, 31 insertions(+), 35 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index bb5544c3e..ceb387b76 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -504,7 +504,7 @@ async def run_update() -> None: f"known updates: [{' '.join(known_updates)}]" ) self._in_progress_updates[job.id] = HandlerExecution( - job.name, defn.unfinished_handlers_policy, job.id + job.name, defn.unfinished_policy, job.id ) args = self._process_handler_args( job.name, @@ -1688,7 +1688,7 @@ def _process_signal_job( self._handled_signals_seq += 1 id = self._handled_signals_seq self._in_progress_signals[id] = HandlerExecution( - job.signal_name, defn.unfinished_handlers_policy + job.signal_name, defn.unfinished_policy ) def done_callback(f): diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 0e2cdb22f..e88838d0c 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -195,7 +195,7 @@ def signal(fn: CallableSyncOrAsyncReturnNoneType) -> CallableSyncOrAsyncReturnNo @overload def signal( *, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -204,7 +204,7 @@ def signal( def signal( *, name: str, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -213,7 +213,7 @@ def signal( def signal( *, dynamic: Literal[True], - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -223,7 +223,7 @@ def signal( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow signal method. @@ -244,13 +244,13 @@ def signal( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - unfinished_handlers_policy: Actions taken if a workflow terminates with + unfinished_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - unfinished_handlers_policy: UnfinishedHandlersPolicy, + unfinished_policy: UnfinishedHandlersPolicy, fn: CallableSyncOrAsyncReturnNoneType, ) -> CallableSyncOrAsyncReturnNoneType: if not name and not dynamic: @@ -259,7 +259,7 @@ def decorator( name=name, fn=fn, is_method=True, - unfinished_handlers_policy=unfinished_handlers_policy, + unfinished_policy=unfinished_policy, ) setattr(fn, "__temporal_signal_definition", defn) if defn.dynamic_vararg: @@ -273,9 +273,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, unfinished_handlers_policy) + return partial(decorator, name, unfinished_policy) else: - return decorator(fn.__name__, unfinished_handlers_policy, fn) + return decorator(fn.__name__, unfinished_policy, fn) @overload @@ -965,7 +965,7 @@ def update( @overload def update( *, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -977,7 +977,7 @@ def update( def update( *, name: str, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -989,7 +989,7 @@ def update( def update( *, dynamic: Literal[True], - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1002,7 +1002,7 @@ def update( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_handlers_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow update handler method. @@ -1030,13 +1030,13 @@ def update( parameters of the method must be self, a string name, and a ``*args`` positional varargs. Cannot be present when ``name`` is present. - unfinished_handlers_policy: Actions taken if a workflow terminates with + unfinished_policy: Actions taken if a workflow terminates with a running instance of this handler. """ def decorator( name: Optional[str], - unfinished_handlers_policy: UnfinishedHandlersPolicy, + unfinished_policy: UnfinishedHandlersPolicy, fn: CallableSyncOrAsyncType, ) -> CallableSyncOrAsyncType: if not name and not dynamic: @@ -1045,7 +1045,7 @@ def decorator( name=name, fn=fn, is_method=True, - unfinished_handlers_policy=unfinished_handlers_policy, + unfinished_policy=unfinished_policy, ) if defn.dynamic_vararg: raise RuntimeError( @@ -1058,9 +1058,9 @@ def decorator( if not fn: if name is not None and dynamic: raise RuntimeError("Cannot provide name and dynamic boolean") - return partial(decorator, name, unfinished_handlers_policy) + return partial(decorator, name, unfinished_policy) else: - return decorator(fn.__name__, unfinished_handlers_policy, fn) + return decorator(fn.__name__, unfinished_policy, fn) def _update_validator( @@ -1517,7 +1517,7 @@ class _SignalDefinition: name: Optional[str] fn: Callable[..., Union[None, Awaitable[None]]] is_method: bool - unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + unfinished_policy: UnfinishedHandlersPolicy = ( UnfinishedHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None @@ -1601,7 +1601,7 @@ class _UpdateDefinition: name: Optional[str] fn: Callable[..., Union[Any, Awaitable[Any]]] is_method: bool - unfinished_handlers_policy: UnfinishedHandlersPolicy = ( + unfinished_policy: UnfinishedHandlersPolicy = ( UnfinishedHandlersPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index a4f8525a3..111e97c89 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5203,14 +5203,12 @@ async def _do_update_or_signal(self) -> None: async def my_update(self) -> None: await self._do_update_or_signal() - @workflow.update( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON - ) + @workflow.update(unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON) async def my_update_ABANDON(self) -> None: await self._do_update_or_signal() @workflow.update( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON ) async def my_update_WARN_AND_ABANDON(self) -> None: await self._do_update_or_signal() @@ -5219,14 +5217,12 @@ async def my_update_WARN_AND_ABANDON(self) -> None: async def my_signal(self): await self._do_update_or_signal() - @workflow.signal( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON - ) + @workflow.signal(unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON) async def my_signal_ABANDON(self): await self._do_update_or_signal() @workflow.signal( - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON ) async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() @@ -5257,7 +5253,7 @@ async def run_test(self): # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, ) assert not handler_finished and warning # but not when the workflow waits for handlers to complete, @@ -5268,14 +5264,14 @@ async def run_test(self): # nor when the silence-warnings policy is set on the handler. handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON, + unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON, ) assert not handler_finished and not warning async def get_workflow_result_and_warning( self, wait_for_handlers: bool, - unfinished_handlers_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, + unfinished_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, ) -> Tuple[bool, bool]: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, @@ -5284,8 +5280,8 @@ async def get_workflow_result_and_warning( task_queue=self.worker.task_queue, ) handler_name = f"my_{self.handler_type}" - if unfinished_handlers_policy: - handler_name += f"_{unfinished_handlers_policy.name}" + if unfinished_policy: + handler_name += f"_{unfinished_policy.name}" with pytest.WarningsRecorder() as warnings: if self.handler_type == "signal": await asyncio.gather(handle.signal(handler_name)) From 8e1c30e6e693697752f98395817b175ec6015bcc Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 12:57:17 -0400 Subject: [PATCH 23/45] Drive-by removal of unused imports --- tests/worker/test_workflow.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 111e97c89..f32b0ba67 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -110,10 +110,8 @@ new_worker, workflow_update_exists, ) -from tests.helpers.external_coroutine import wait_on_timer from tests.helpers.external_stack_trace import ( ExternalStackTraceWorkflow, - MultiFileStackTraceWorkflow, external_wait_cancel, ) From 1b42175ef73a29a6e2857c82a5264cf22688184c Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 12:57:42 -0400 Subject: [PATCH 24/45] Use custom warnings in test --- tests/worker/test_workflow.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index f32b0ba67..19ca9ed42 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -103,6 +103,10 @@ WorkflowInstanceDetails, WorkflowRunner, ) +from temporalio.worker._workflow_instance import ( + UnfinishedSignalHandlerWarning, + UnfinishedUpdateHandlerWarning, +) from tests.helpers import ( assert_eq_eventually, ensure_search_attributes_present, @@ -5301,17 +5305,14 @@ async def get_workflow_result_and_warning( wf_result = await handle.result() unfinished_handler_warning_emitted = any( - self.is_unfinished_handler_warning(w) for w in warnings + issubclass(w.category, self.unfinished_handler_warning_cls) + for w in warnings ) return wf_result, unfinished_handler_warning_emitted - def is_unfinished_handler_warning( - self, - warning: warnings.WarningMessage, - ) -> bool: - expected_text = ( - f"Workflow finished while {self.handler_type} handlers are still running" - ) - return issubclass(warning.category, RuntimeWarning) and expected_text in str( - warning.message - ) + @property + def unfinished_handler_warning_cls(self) -> Type: + return { + "update": UnfinishedUpdateHandlerWarning, + "signal": UnfinishedSignalHandlerWarning, + }[self.handler_type] From 4acc28bfb6d8fd54d0e374e30be97538e436a0e1 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 12:56:13 -0400 Subject: [PATCH 25/45] Convert unfinished handler warnings to errors during test suite --- pyproject.toml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 021fb0d79..4bf1d844d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -99,7 +99,6 @@ env = { TEMPORAL_INTEGRATION_TEST = "1" } cmd = "pip uninstall temporalio -y" [tool.pytest.ini_options] -addopts = "-p no:warnings" asyncio_mode = "auto" log_cli = true log_cli_level = "INFO" @@ -107,6 +106,12 @@ log_cli_format = "%(asctime)s [%(levelname)8s] %(message)s (%(filename)s:%(linen testpaths = ["tests"] timeout = 600 timeout_func_only = true +filterwarnings = [ + "error::temporalio.worker._workflow_instance.UnfinishedUpdateHandlerWarning", + "error::temporalio.worker._workflow_instance.UnfinishedSignalHandlerWarning", + "ignore::pytest.PytestDeprecationWarning", + "ignore::DeprecationWarning", +] [tool.cibuildwheel] # We only want the 3.8 64-bit build of each type. However, due to From 48fb62248a8ab6d312c0eaa370278fd9a7776f7d Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 13:53:12 -0400 Subject: [PATCH 26/45] Satisfy pydocstyle linter --- temporalio/worker/_workflow_instance.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index ceb387b76..a02892ce2 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -2739,17 +2739,23 @@ class _WorkflowBeingEvictedError(BaseException): @dataclass class HandlerExecution: + """Information about an execution of a signal or update handler.""" + name: str unfinished_handlers_policy: temporalio.workflow.UnfinishedHandlersPolicy id: Optional[str] = None class UnfinishedUpdateHandlerWarning(RuntimeWarning): + """Warning issued when a workflow exits before an update handler has finished executing""" + def __init__(self, handler_executions: List[HandlerExecution]) -> None: + """Initialize warning object""" super().__init__() self.handler_executions = handler_executions def __str__(self) -> str: + """Return warning message""" message = """ Workflow finished while update handlers are still running. This may have interrupted work that the update handler was doing, and the client that sent the update will receive a 'workflow execution @@ -2771,11 +2777,15 @@ def __str__(self) -> str: class UnfinishedSignalHandlerWarning(RuntimeWarning): + """Warning issued when a workflow exits before a signal handler has finished executing""" + def __init__(self, handler_executions: List[HandlerExecution]) -> None: + """Initialize warning object""" super().__init__() self.handler_executions = handler_executions def __str__(self) -> str: + """Return warning message""" message = """ Workflow finished while signal handlers are still running. This may have interrupted work that the signal handler was doing. You can wait for all update and signal handlers to complete by using From 67e561121b703f5c368dc5a585c96b37556dbaaa Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 14:14:56 -0400 Subject: [PATCH 27/45] Use unfinished_policy=HandlerUnfinishedPolicy.XXX --- temporalio/worker/_workflow_instance.py | 10 ++++----- temporalio/workflow.py | 30 ++++++++++++------------- tests/worker/test_workflow.py | 16 ++++++------- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index a02892ce2..2cbe7ae48 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -1622,8 +1622,8 @@ def warnable(handler_executions: Iterable[HandlerExecution]): return [ ex for ex in handler_executions - if ex.unfinished_handlers_policy - == temporalio.workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + if ex.unfinished_policy + == temporalio.workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON ] warnable_updates = warnable(self._in_progress_updates.values()) @@ -2742,7 +2742,7 @@ class HandlerExecution: """Information about an execution of a signal or update handler.""" name: str - unfinished_handlers_policy: temporalio.workflow.UnfinishedHandlersPolicy + unfinished_policy: temporalio.workflow.HandlerUnfinishedPolicy id: Optional[str] = None @@ -2764,7 +2764,7 @@ def __str__(self) -> str: Alternatively, if both you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the update handler decorator: -`@workflow.update(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +`@workflow.update(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( "\n", " " ).strip() @@ -2793,7 +2793,7 @@ def __str__(self) -> str: you and the clients sending updates and signals are okay with interrupting running handlers when the workflow finishes, and causing clients to receive errors, then you can disable this warning via the signal handler decorator: -`@workflow.signal(unfinished_handlers_policy=workflow.UnfinishedHandlersPolicy.ABANDON)`. +`@workflow.signal(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( "\n", " " ).strip() diff --git a/temporalio/workflow.py b/temporalio/workflow.py index e88838d0c..bcaba1a4c 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -173,7 +173,7 @@ def run(fn: CallableAsyncType) -> CallableAsyncType: return fn # type: ignore[return-value] -class UnfinishedHandlersPolicy(Enum): +class HandlerUnfinishedPolicy(Enum): """Actions taken if a workflow terminates with running handlers. Policy defining actions taken when a workflow exits while update or signal handlers are running. @@ -195,7 +195,7 @@ def signal(fn: CallableSyncOrAsyncReturnNoneType) -> CallableSyncOrAsyncReturnNo @overload def signal( *, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -204,7 +204,7 @@ def signal( def signal( *, name: str, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -213,7 +213,7 @@ def signal( def signal( *, dynamic: Literal[True], - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[[CallableSyncOrAsyncReturnNoneType], CallableSyncOrAsyncReturnNoneType]: ... @@ -223,7 +223,7 @@ def signal( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow signal method. @@ -250,7 +250,7 @@ def signal( def decorator( name: Optional[str], - unfinished_policy: UnfinishedHandlersPolicy, + unfinished_policy: HandlerUnfinishedPolicy, fn: CallableSyncOrAsyncReturnNoneType, ) -> CallableSyncOrAsyncReturnNoneType: if not name and not dynamic: @@ -965,7 +965,7 @@ def update( @overload def update( *, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -977,7 +977,7 @@ def update( def update( *, name: str, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -989,7 +989,7 @@ def update( def update( *, dynamic: Literal[True], - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) -> Callable[ [Callable[MultiParamSpec, ReturnType]], UpdateMethodMultiParam[MultiParamSpec, ReturnType], @@ -1002,7 +1002,7 @@ def update( *, name: Optional[str] = None, dynamic: Optional[bool] = False, - unfinished_policy: UnfinishedHandlersPolicy = UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy: HandlerUnfinishedPolicy = HandlerUnfinishedPolicy.WARN_AND_ABANDON, ): """Decorator for a workflow update handler method. @@ -1036,7 +1036,7 @@ def update( def decorator( name: Optional[str], - unfinished_policy: UnfinishedHandlersPolicy, + unfinished_policy: HandlerUnfinishedPolicy, fn: CallableSyncOrAsyncType, ) -> CallableSyncOrAsyncType: if not name and not dynamic: @@ -1517,8 +1517,8 @@ class _SignalDefinition: name: Optional[str] fn: Callable[..., Union[None, Awaitable[None]]] is_method: bool - unfinished_policy: UnfinishedHandlersPolicy = ( - UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy: HandlerUnfinishedPolicy = ( + HandlerUnfinishedPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None @@ -1601,8 +1601,8 @@ class _UpdateDefinition: name: Optional[str] fn: Callable[..., Union[Any, Awaitable[Any]]] is_method: bool - unfinished_policy: UnfinishedHandlersPolicy = ( - UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy: HandlerUnfinishedPolicy = ( + HandlerUnfinishedPolicy.WARN_AND_ABANDON ) # Types loaded on post init if None arg_types: Optional[List[Type]] = None diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 19ca9ed42..c7dfc6c8e 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5205,12 +5205,12 @@ async def _do_update_or_signal(self) -> None: async def my_update(self) -> None: await self._do_update_or_signal() - @workflow.update(unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON) + @workflow.update(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON) async def my_update_ABANDON(self) -> None: await self._do_update_or_signal() @workflow.update( - unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy=workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON ) async def my_update_WARN_AND_ABANDON(self) -> None: await self._do_update_or_signal() @@ -5219,12 +5219,12 @@ async def my_update_WARN_AND_ABANDON(self) -> None: async def my_signal(self): await self._do_update_or_signal() - @workflow.signal(unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON) + @workflow.signal(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON) async def my_signal_ABANDON(self): await self._do_update_or_signal() @workflow.signal( - unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON + unfinished_policy=workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON ) async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() @@ -5252,10 +5252,10 @@ async def run_test(self): wait_for_handlers=False, ) assert not handler_finished and warning - # and when the workflow sets the unfinished_handlers_policy to WARN_AND_ABANDON, + # and when the workflow sets the unfinished_policy to WARN_AND_ABANDON, handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_policy=workflow.UnfinishedHandlersPolicy.WARN_AND_ABANDON, + unfinished_policy=workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) assert not handler_finished and warning # but not when the workflow waits for handlers to complete, @@ -5266,14 +5266,14 @@ async def run_test(self): # nor when the silence-warnings policy is set on the handler. handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, - unfinished_policy=workflow.UnfinishedHandlersPolicy.ABANDON, + unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON, ) assert not handler_finished and not warning async def get_workflow_result_and_warning( self, wait_for_handlers: bool, - unfinished_policy: Optional[workflow.UnfinishedHandlersPolicy] = None, + unfinished_policy: Optional[workflow.HandlerUnfinishedPolicy] = None, ) -> Tuple[bool, bool]: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, From f0b17396746f75d74134d2451c333a500de8d0e6 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 14:21:46 -0400 Subject: [PATCH 28/45] Test that unfinished handlers cause exceptions in the test suite --- tests/worker/test_workflow.py | 84 ++++++++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 22 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index c7dfc6c8e..f8735a6e5 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -11,9 +11,10 @@ import warnings from abc import ABC, abstractmethod from contextlib import contextmanager -from dataclasses import dataclass +from dataclasses import dataclass, field from datetime import datetime, timedelta, timezone from enum import IntEnum +from functools import partial from typing import ( Any, Awaitable, @@ -5247,6 +5248,19 @@ class _UnfinishedHandlersTest: handler_type: Literal["update", "signal"] async def run_test(self): + # If we don't capture warnings then -- since the unfinished handler warning is converted to + # an exception in the test suite -- we see WFT failures when we don't wait for handlers. + handle: asyncio.Future[WorkflowHandle] = asyncio.Future() + asyncio.create_task( + self.get_workflow_result(wait_for_handlers=False, handle_future=handle) + ) + await assert_eq_eventually( + True, + partial(self.workflow_task_failed, workflow_id=(await handle).id), + timeout=timedelta(seconds=5), + interval=timedelta(seconds=1), + ) + # The unfinished handler warning is issued by default, handler_finished, warning = await self.get_workflow_result_and_warning( wait_for_handlers=False, @@ -5270,45 +5284,71 @@ async def run_test(self): ) assert not handler_finished and not warning + async def workflow_task_failed(self, workflow_id: str) -> bool: + resp = await self.client.workflow_service.get_workflow_execution_history( + GetWorkflowExecutionHistoryRequest( + namespace=self.client.namespace, + execution=WorkflowExecution(workflow_id=workflow_id), + ), + ) + for event in reversed(resp.history.events): + if event.event_type == EventType.EVENT_TYPE_WORKFLOW_TASK_FAILED: + assert event.workflow_task_failed_event_attributes.failure.message.startswith( + f"Workflow finished while {self.handler_type} handlers are still running" + ) + return True + return False + async def get_workflow_result_and_warning( self, wait_for_handlers: bool, unfinished_policy: Optional[workflow.HandlerUnfinishedPolicy] = None, ) -> Tuple[bool, bool]: + with pytest.WarningsRecorder() as warnings: + wf_result = await self.get_workflow_result( + wait_for_handlers, unfinished_policy + ) + unfinished_handler_warning_emitted = any( + issubclass(w.category, self.unfinished_handler_warning_cls) + for w in warnings + ) + return wf_result, unfinished_handler_warning_emitted + + async def get_workflow_result( + self, + wait_for_handlers: bool, + unfinished_policy: Optional[workflow.HandlerUnfinishedPolicy] = None, + handle_future: Optional[asyncio.Future[WorkflowHandle]] = None, + ) -> bool: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, arg=wait_for_handlers, id=f"wf-{uuid.uuid4()}", task_queue=self.worker.task_queue, ) + if handle_future: + handle_future.set_result(handle) handler_name = f"my_{self.handler_type}" if unfinished_policy: handler_name += f"_{unfinished_policy.name}" - with pytest.WarningsRecorder() as warnings: - if self.handler_type == "signal": - await asyncio.gather(handle.signal(handler_name)) - else: - if not wait_for_handlers: - with pytest.raises(RPCError) as err: - await asyncio.gather( - handle.execute_update(handler_name, id="my-update") - ) - assert ( - err.value.status == RPCStatusCode.NOT_FOUND - and "workflow execution already completed" - in str(err.value).lower() - ) - else: + if self.handler_type == "signal": + await asyncio.gather(handle.signal(handler_name)) + else: + if not wait_for_handlers: + with pytest.raises(RPCError) as err: await asyncio.gather( handle.execute_update(handler_name, id="my-update") ) + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" in str(err.value).lower() + ) + else: + await asyncio.gather( + handle.execute_update(handler_name, id="my-update") + ) - wf_result = await handle.result() - unfinished_handler_warning_emitted = any( - issubclass(w.category, self.unfinished_handler_warning_cls) - for w in warnings - ) - return wf_result, unfinished_handler_warning_emitted + return await handle.result() @property def unfinished_handler_warning_cls(self) -> Type: From 572cf9ab54cbf8f7ccc5a4fb827ca0c58bded34f Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 17:23:40 -0400 Subject: [PATCH 29/45] Allow use of generics with asyncio.Future in Python 3.8 --- tests/worker/test_workflow.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index f8735a6e5..50fd31e15 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import asyncio import dataclasses import json From 04373d85dcc6bb35ea08ca18380a030fdb36825b Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 17:56:35 -0400 Subject: [PATCH 30/45] Move warning classes to temporalio.workflow --- pyproject.toml | 4 +- temporalio/worker/_workflow_instance.py | 76 +++++++++++-------------- temporalio/workflow.py | 8 +++ tests/worker/test_workflow.py | 8 +-- 4 files changed, 46 insertions(+), 50 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 4bf1d844d..41f010aed 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,8 +107,8 @@ testpaths = ["tests"] timeout = 600 timeout_func_only = true filterwarnings = [ - "error::temporalio.worker._workflow_instance.UnfinishedUpdateHandlerWarning", - "error::temporalio.worker._workflow_instance.UnfinishedSignalHandlerWarning", + "error::temporalio.workflow.UnfinishedUpdateHandlerWarning", + "error::temporalio.workflow.UnfinishedSignalHandlerWarning", "ignore::pytest.PytestDeprecationWarning", "ignore::DeprecationWarning", ] diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 2cbe7ae48..b596a8b61 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -1628,11 +1628,19 @@ def warnable(handler_executions: Iterable[HandlerExecution]): warnable_updates = warnable(self._in_progress_updates.values()) if warnable_updates: - warnings.warn(UnfinishedUpdateHandlerWarning(warnable_updates)) + warnings.warn( + temporalio.workflow.UnfinishedUpdateHandlerWarning( + _make_unfinished_update_handler_message(warnable_updates) + ) + ) warnable_signals = warnable(self._in_progress_signals.values()) if warnable_signals: - warnings.warn(UnfinishedSignalHandlerWarning(warnable_signals)) + warnings.warn( + temporalio.workflow.UnfinishedSignalHandlerWarning( + _make_unfinished_signal_handler_message(warnable_signals) + ) + ) def _next_seq(self, type: str) -> int: seq = self._curr_seqs.get(type, 0) + 1 @@ -2746,17 +2754,10 @@ class HandlerExecution: id: Optional[str] = None -class UnfinishedUpdateHandlerWarning(RuntimeWarning): - """Warning issued when a workflow exits before an update handler has finished executing""" - - def __init__(self, handler_executions: List[HandlerExecution]) -> None: - """Initialize warning object""" - super().__init__() - self.handler_executions = handler_executions - - def __str__(self) -> str: - """Return warning message""" - message = """ +def _make_unfinished_update_handler_message( + handler_executions: List[HandlerExecution], +) -> str: + message = """ Workflow finished while update handlers are still running. This may have interrupted work that the update handler was doing, and the client that sent the update will receive a 'workflow execution already completed' RPCError instead of the update result. You can wait for all update and signal @@ -2766,27 +2767,18 @@ def __str__(self) -> str: disable this warning via the update handler decorator: `@workflow.update(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( - "\n", " " - ).strip() - return ( - f"{message} The following updates were unfinished (and warnings were not disabled for their handler): " - + json.dumps( - [{"name": ex.name, "id": ex.id} for ex in self.handler_executions] - ) - ) - - -class UnfinishedSignalHandlerWarning(RuntimeWarning): - """Warning issued when a workflow exits before a signal handler has finished executing""" - - def __init__(self, handler_executions: List[HandlerExecution]) -> None: - """Initialize warning object""" - super().__init__() - self.handler_executions = handler_executions - - def __str__(self) -> str: - """Return warning message""" - message = """ + "\n", " " + ).strip() + return ( + f"{message} The following updates were unfinished (and warnings were not disabled for their handler): " + + json.dumps([{"name": ex.name, "id": ex.id} for ex in handler_executions]) + ) + + +def _make_unfinished_signal_handler_message( + handler_executions: List[HandlerExecution], +) -> str: + message = """ Workflow finished while signal handlers are still running. This may have interrupted work that the signal handler was doing. You can wait for all update and signal handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both @@ -2795,12 +2787,12 @@ def __str__(self) -> str: signal handler decorator: `@workflow.signal(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( - "\n", " " - ).strip() - names = collections.Counter(ex.name for ex in self.handler_executions) - return ( - f"{message} The following signals were unfinished (and warnings were not disabled for their handler): " - + json.dumps( - [{"name": name, "count": count} for name, count in names.most_common()] - ) + "\n", " " + ).strip() + names = collections.Counter(ex.name for ex in handler_executions) + return ( + f"{message} The following signals were unfinished (and warnings were not disabled for their handler): " + + json.dumps( + [{"name": name, "count": count} for name, count in names.most_common()] ) + ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index bcaba1a4c..a9f4698f4 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -4729,3 +4729,11 @@ def _to_proto(self) -> temporalio.bridge.proto.common.VersioningIntent.ValueType elif self == VersioningIntent.DEFAULT: return temporalio.bridge.proto.common.VersioningIntent.DEFAULT return temporalio.bridge.proto.common.VersioningIntent.UNSPECIFIED + + +class UnfinishedUpdateHandlerWarning(RuntimeWarning): + """Warning issued when a workflow exits before an update handler has finished executing""" + + +class UnfinishedSignalHandlerWarning(RuntimeWarning): + """Warning issued when a workflow exits before a signal handler has finished executing""" diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 50fd31e15..09a6b03ab 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -106,10 +106,6 @@ WorkflowInstanceDetails, WorkflowRunner, ) -from temporalio.worker._workflow_instance import ( - UnfinishedSignalHandlerWarning, - UnfinishedUpdateHandlerWarning, -) from tests.helpers import ( assert_eq_eventually, ensure_search_attributes_present, @@ -5355,6 +5351,6 @@ async def get_workflow_result( @property def unfinished_handler_warning_cls(self) -> Type: return { - "update": UnfinishedUpdateHandlerWarning, - "signal": UnfinishedSignalHandlerWarning, + "update": workflow.UnfinishedUpdateHandlerWarning, + "signal": workflow.UnfinishedSignalHandlerWarning, }[self.handler_type] From b1ee63056f85b2485507c83f0c3d97ce76d71208 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 18:09:56 -0400 Subject: [PATCH 31/45] Pluralize warning names and fix docstrings --- pyproject.toml | 4 ++-- temporalio/worker/_workflow_instance.py | 4 ++-- temporalio/workflow.py | 17 +++++++++-------- tests/worker/test_workflow.py | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 41f010aed..e80f4fcf6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,8 +107,8 @@ testpaths = ["tests"] timeout = 600 timeout_func_only = true filterwarnings = [ - "error::temporalio.workflow.UnfinishedUpdateHandlerWarning", - "error::temporalio.workflow.UnfinishedSignalHandlerWarning", + "error::temporalio.workflow.UnfinishedUpdateHandlersWarning", + "error::temporalio.workflow.UnfinishedSignalHandlersWarning", "ignore::pytest.PytestDeprecationWarning", "ignore::DeprecationWarning", ] diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index b596a8b61..5e0ff1339 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -1629,7 +1629,7 @@ def warnable(handler_executions: Iterable[HandlerExecution]): warnable_updates = warnable(self._in_progress_updates.values()) if warnable_updates: warnings.warn( - temporalio.workflow.UnfinishedUpdateHandlerWarning( + temporalio.workflow.UnfinishedUpdateHandlersWarning( _make_unfinished_update_handler_message(warnable_updates) ) ) @@ -1637,7 +1637,7 @@ def warnable(handler_executions: Iterable[HandlerExecution]): warnable_signals = warnable(self._in_progress_signals.values()) if warnable_signals: warnings.warn( - temporalio.workflow.UnfinishedSignalHandlerWarning( + temporalio.workflow.UnfinishedSignalHandlersWarning( _make_unfinished_signal_handler_message(warnable_signals) ) ) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index a9f4698f4..83051b31f 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -187,6 +187,15 @@ class HandlerUnfinishedPolicy(Enum): ABANDON = 2 + +class UnfinishedUpdateHandlersWarning(RuntimeWarning): + """The workflow exited before all update handlers had finished executing.""" + + +class UnfinishedSignalHandlersWarning(RuntimeWarning): + """The workflow exited before all signal handlers had finished executing.""" + + @overload def signal(fn: CallableSyncOrAsyncReturnNoneType) -> CallableSyncOrAsyncReturnNoneType: ... @@ -4729,11 +4738,3 @@ def _to_proto(self) -> temporalio.bridge.proto.common.VersioningIntent.ValueType elif self == VersioningIntent.DEFAULT: return temporalio.bridge.proto.common.VersioningIntent.DEFAULT return temporalio.bridge.proto.common.VersioningIntent.UNSPECIFIED - - -class UnfinishedUpdateHandlerWarning(RuntimeWarning): - """Warning issued when a workflow exits before an update handler has finished executing""" - - -class UnfinishedSignalHandlerWarning(RuntimeWarning): - """Warning issued when a workflow exits before a signal handler has finished executing""" diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 09a6b03ab..8beec874e 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5351,6 +5351,6 @@ async def get_workflow_result( @property def unfinished_handler_warning_cls(self) -> Type: return { - "update": workflow.UnfinishedUpdateHandlerWarning, - "signal": workflow.UnfinishedSignalHandlerWarning, + "update": workflow.UnfinishedUpdateHandlersWarning, + "signal": workflow.UnfinishedSignalHandlersWarning, }[self.handler_type] From 3e2181f4336d5aac07e5416169fc306def4d7b37 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 18:11:01 -0400 Subject: [PATCH 32/45] Turn enum comments into docstrings --- temporalio/workflow.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/temporalio/workflow.py b/temporalio/workflow.py index 83051b31f..3d98df9f6 100644 --- a/temporalio/workflow.py +++ b/temporalio/workflow.py @@ -180,12 +180,13 @@ class HandlerUnfinishedPolicy(Enum): The workflow exit may be due to successful return, failure, cancellation, or continue-as-new. """ - # Issue a warning in addition to abandoning. WARN_AND_ABANDON = 1 - # Abandon the handler. In the case of an update handler this means that the client will receive - # an error rather than the update result. + """Issue a warning in addition to abandoning.""" ABANDON = 2 + """Abandon the handler. + In the case of an update handler this means that the client will receive an error rather than + the update result.""" class UnfinishedUpdateHandlersWarning(RuntimeWarning): From fcabe2100debdd33b9c698b4b0b20306ce84ec5d Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 18:36:52 -0400 Subject: [PATCH 33/45] Use default timeout and interval --- tests/worker/test_workflow.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 8beec874e..333398dfd 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5253,10 +5253,7 @@ async def run_test(self): self.get_workflow_result(wait_for_handlers=False, handle_future=handle) ) await assert_eq_eventually( - True, - partial(self.workflow_task_failed, workflow_id=(await handle).id), - timeout=timedelta(seconds=5), - interval=timedelta(seconds=1), + True, partial(self.workflow_task_failed, workflow_id=(await handle).id) ) # The unfinished handler warning is issued by default, From ea6392089c3f6117b98dcc64080dc7eb06404573 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Fri, 21 Jun 2024 18:37:12 -0400 Subject: [PATCH 34/45] Clean up imports --- tests/worker/test_workflow.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 333398dfd..d38ac1e75 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -10,10 +10,9 @@ import threading import typing import uuid -import warnings from abc import ABC, abstractmethod from contextlib import contextmanager -from dataclasses import dataclass, field +from dataclasses import dataclass from datetime import datetime, timedelta, timezone from enum import IntEnum from functools import partial From 7063c17a46df70fc19603c706b7c312aedaaa780 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Mon, 24 Jun 2024 10:19:44 -0400 Subject: [PATCH 35/45] Edit warning messages --- temporalio/worker/_workflow_instance.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 5e0ff1339..b13a1d7a7 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -2761,10 +2761,10 @@ def _make_unfinished_update_handler_message( Workflow finished while update handlers are still running. This may have interrupted work that the update handler was doing, and the client that sent the update will receive a 'workflow execution already completed' RPCError instead of the update result. You can wait for all update and signal -handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. -Alternatively, if both you and the clients sending updates and signals are okay with interrupting -running handlers when the workflow finishes, and causing clients to receive errors, then you can -disable this warning via the update handler decorator: +handlers to complete by using `await workflow.wait_condition(lambda: +workflow.all_handlers_finished())`. Alternatively, if both you and the clients sending the update +are okay with interrupting running handlers when the workflow finishes, and causing clients to +receive errors, then you can disable this warning via the update handler decorator: `@workflow.update(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( "\n", " " @@ -2782,10 +2782,9 @@ def _make_unfinished_signal_handler_message( Workflow finished while signal handlers are still running. This may have interrupted work that the signal handler was doing. You can wait for all update and signal handlers to complete by using `await workflow.wait_condition(lambda: workflow.all_handlers_finished())`. Alternatively, if both -you and the clients sending updates and signals are okay with interrupting running handlers when the -workflow finishes, and causing clients to receive errors, then you can disable this warning via the -signal handler decorator: -`@workflow.signal(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. +you and the clients sending the signal are okay with interrupting running handlers when the workflow +finishes, and causing clients to receive errors, then you can disable this warning via the signal +handler decorator: `@workflow.signal(unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON)`. """.replace( "\n", " " ).strip() From 8162e31cdd5ff47d1c52b651b4c4c949aa7852ec Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Mon, 24 Jun 2024 13:53:34 -0400 Subject: [PATCH 36/45] Refactor test --- tests/worker/test_workflow.py | 79 +++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index d38ac1e75..c9c606bc2 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5187,9 +5187,9 @@ def __init__(self): self.handler_finished = False @workflow.run - async def run(self, wait_for_handlers: bool) -> bool: + async def run(self, wait_all_handlers_finished: bool) -> bool: await workflow.wait_condition(lambda: self.started_handler) - if wait_for_handlers: + if wait_all_handlers_finished: self.handler_may_return = True await workflow.wait_condition(workflow.all_handlers_finished) return self.handler_finished @@ -5228,14 +5228,18 @@ async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() -async def test_unfinished_update_handlers(client: Client): +async def test_unfinished_update_handler(client: Client): async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - await _UnfinishedHandlersTest(client, worker, "update").run_test() + test = _UnfinishedHandlersTest(client, worker, "update") + await test.test_wait_all_handlers_finished_and_unfinished_handlers_warning() + await test.test_unfinished_handlers_cause_exceptions_in_test_suite() -async def test_unfinished_signal_handlers(client: Client): +async def test_unfinished_signal_handler(client: Client): async with new_worker(client, UnfinishedHandlersWorkflow) as worker: - await _UnfinishedHandlersTest(client, worker, "signal").run_test() + test = _UnfinishedHandlersTest(client, worker, "signal") + await test.test_wait_all_handlers_finished_and_unfinished_handlers_warning() + await test.test_unfinished_handlers_cause_exceptions_in_test_suite() @dataclass @@ -5244,41 +5248,44 @@ class _UnfinishedHandlersTest: worker: Worker handler_type: Literal["update", "signal"] - async def run_test(self): - # If we don't capture warnings then -- since the unfinished handler warning is converted to - # an exception in the test suite -- we see WFT failures when we don't wait for handlers. - handle: asyncio.Future[WorkflowHandle] = asyncio.Future() - asyncio.create_task( - self.get_workflow_result(wait_for_handlers=False, handle_future=handle) - ) - await assert_eq_eventually( - True, partial(self.workflow_task_failed, workflow_id=(await handle).id) - ) - + async def test_wait_all_handlers_finished_and_unfinished_handlers_warning(self): # The unfinished handler warning is issued by default, - handler_finished, warning = await self.get_workflow_result_and_warning( - wait_for_handlers=False, + handler_finished, warning = await self._get_workflow_result_and_warning( + wait_all_handlers_finished=False, ) assert not handler_finished and warning # and when the workflow sets the unfinished_policy to WARN_AND_ABANDON, - handler_finished, warning = await self.get_workflow_result_and_warning( - wait_for_handlers=False, + handler_finished, warning = await self._get_workflow_result_and_warning( + wait_all_handlers_finished=False, unfinished_policy=workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON, ) assert not handler_finished and warning # but not when the workflow waits for handlers to complete, - handler_finished, warning = await self.get_workflow_result_and_warning( - wait_for_handlers=True, + handler_finished, warning = await self._get_workflow_result_and_warning( + wait_all_handlers_finished=True, ) assert handler_finished and not warning # nor when the silence-warnings policy is set on the handler. - handler_finished, warning = await self.get_workflow_result_and_warning( - wait_for_handlers=False, + handler_finished, warning = await self._get_workflow_result_and_warning( + wait_all_handlers_finished=False, unfinished_policy=workflow.HandlerUnfinishedPolicy.ABANDON, ) assert not handler_finished and not warning - async def workflow_task_failed(self, workflow_id: str) -> bool: + async def test_unfinished_handlers_cause_exceptions_in_test_suite(self): + # If we don't capture warnings then -- since the unfinished handler warning is converted to + # an exception in the test suite -- we see WFT failures when we don't wait for handlers. + handle: asyncio.Future[WorkflowHandle] = asyncio.Future() + asyncio.create_task( + self._get_workflow_result( + wait_all_handlers_finished=False, handle_future=handle + ) + ) + await assert_eq_eventually( + True, partial(self._workflow_task_failed, workflow_id=(await handle).id) + ) + + async def _workflow_task_failed(self, workflow_id: str) -> bool: resp = await self.client.workflow_service.get_workflow_execution_history( GetWorkflowExecutionHistoryRequest( namespace=self.client.namespace, @@ -5293,30 +5300,30 @@ async def workflow_task_failed(self, workflow_id: str) -> bool: return True return False - async def get_workflow_result_and_warning( + async def _get_workflow_result_and_warning( self, - wait_for_handlers: bool, + wait_all_handlers_finished: bool, unfinished_policy: Optional[workflow.HandlerUnfinishedPolicy] = None, ) -> Tuple[bool, bool]: with pytest.WarningsRecorder() as warnings: - wf_result = await self.get_workflow_result( - wait_for_handlers, unfinished_policy + wf_result = await self._get_workflow_result( + wait_all_handlers_finished, unfinished_policy ) unfinished_handler_warning_emitted = any( - issubclass(w.category, self.unfinished_handler_warning_cls) + issubclass(w.category, self._unfinished_handler_warning_cls) for w in warnings ) return wf_result, unfinished_handler_warning_emitted - async def get_workflow_result( + async def _get_workflow_result( self, - wait_for_handlers: bool, + wait_all_handlers_finished: bool, unfinished_policy: Optional[workflow.HandlerUnfinishedPolicy] = None, handle_future: Optional[asyncio.Future[WorkflowHandle]] = None, ) -> bool: handle = await self.client.start_workflow( UnfinishedHandlersWorkflow.run, - arg=wait_for_handlers, + arg=wait_all_handlers_finished, id=f"wf-{uuid.uuid4()}", task_queue=self.worker.task_queue, ) @@ -5328,7 +5335,7 @@ async def get_workflow_result( if self.handler_type == "signal": await asyncio.gather(handle.signal(handler_name)) else: - if not wait_for_handlers: + if not wait_all_handlers_finished: with pytest.raises(RPCError) as err: await asyncio.gather( handle.execute_update(handler_name, id="my-update") @@ -5345,7 +5352,7 @@ async def get_workflow_result( return await handle.result() @property - def unfinished_handler_warning_cls(self) -> Type: + def _unfinished_handler_warning_cls(self) -> Type: return { "update": workflow.UnfinishedUpdateHandlersWarning, "signal": workflow.UnfinishedSignalHandlersWarning, From 15ed853858bd136f22eb354945be7ff1d56e882b Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 05:13:06 -0400 Subject: [PATCH 37/45] Test cancellation --- tests/worker/test_workflow.py | 99 +++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index c9c606bc2..405c7fef4 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5357,3 +5357,102 @@ def _unfinished_handler_warning_cls(self) -> Type: "update": workflow.UnfinishedUpdateHandlersWarning, "signal": workflow.UnfinishedSignalHandlersWarning, }[self.handler_type] + + +@workflow.defn +class UnfinishedHandlersWithCancellationWorkflow: + @workflow.run + async def run(self) -> NoReturn: + await workflow.wait_condition(lambda: False) + + @workflow.update + async def my_update(self) -> str: + await workflow.wait_condition(lambda: False) + return "update-result" + + @workflow.signal + async def my_signal(self): + await workflow.wait_condition(lambda: False) + + +async def test_unfinished_update_handler_with_workflow_cancellation(client: Client): + await _UnfinishedHandlersWithCancellationTest( + client, "update" + ).test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler() + + +async def test_unfinished_signal_handler_with_workflow_cancellation(client: Client): + await _UnfinishedHandlersWithCancellationTest( + client, "signal" + ).test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler() + + +@dataclass +class _UnfinishedHandlersWithCancellationTest: + client: Client + handler_type: Literal["update", "signal"] + + async def test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler( + self, + ): + assert await self._run_workflow_and_get_warning() + + async def _run_workflow_and_get_warning(self) -> bool: + workflow_id = f"wf-{uuid.uuid4()}" + update_id = "update-id" + task_queue = "tq" + + # We require a cancellation request and an update to be delivered in the same WFT. To do + # this we send the start, cancel, and update/signal requests, and then start the worker + # after they've all been accepted by the server. + handle = await self.client.start_workflow( + UnfinishedHandlersWithCancellationWorkflow.run, + id=workflow_id, + task_queue=task_queue, + ) + await handle.cancel() + + if self.handler_type == "update": + update_task = asyncio.create_task( + handle.execute_update( + UnfinishedHandlersWithCancellationWorkflow.my_update, id=update_id + ) + ) + await assert_eq_eventually( + True, lambda: workflow_update_exists(self.client, workflow_id, update_id) + ) + else: + await handle.signal(UnfinishedHandlersWithCancellationWorkflow.my_signal) + + async with new_worker( + self.client, + UnfinishedHandlersWithCancellationWorkflow, + task_queue=task_queue, + ): + with pytest.WarningsRecorder() as warnings: + if self.handler_type == "update": + assert update_task + with pytest.raises(RPCError) as err: + await update_task + assert ( + err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" + in str(err.value).lower() + ) + + with pytest.raises(WorkflowFailureError) as err: + await handle.result() + assert "workflow execution failed" in str(err.value).lower() + + unfinished_handler_warning_emitted = any( + issubclass(w.category, self._unfinished_handler_warning_cls) + for w in warnings + ) + return unfinished_handler_warning_emitted + + @property + def _unfinished_handler_warning_cls(self) -> Type: + return { + "update": workflow.UnfinishedUpdateHandlersWarning, + "signal": workflow.UnfinishedSignalHandlersWarning, + }[self.handler_type] From 3f0b11c9523943ea6403061fe7f8d9082e279425 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 10:05:21 -0400 Subject: [PATCH 38/45] Bug fix: include cancellation command in set of "completion" commands --- temporalio/worker/_workflow_instance.py | 1 + 1 file changed, 1 insertion(+) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index b13a1d7a7..b0cff9ffd 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -417,6 +417,7 @@ def activate( command.HasField("complete_workflow_execution") or command.HasField("continue_as_new_workflow_execution") or command.HasField("fail_workflow_execution") + or command.HasField("cancel_workflow_execution") ) elif not command.HasField("respond_to_query"): del self._current_completion.successful.commands[i] From d6ca2466f1261caccc56cb6fc797866da0286893 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 11:42:12 -0400 Subject: [PATCH 39/45] Add coverage for workflow failure --- tests/worker/test_workflow.py | 71 +++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 405c7fef4..2ba56ab24 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5360,9 +5360,15 @@ def _unfinished_handler_warning_cls(self) -> Type: @workflow.defn -class UnfinishedHandlersWithCancellationWorkflow: +class UnfinishedHandlersWithCancellationOrFailureWorkflow: @workflow.run - async def run(self) -> NoReturn: + async def run( + self, workflow_termination_type: Literal["cancellation", "failure"] + ) -> NoReturn: + if workflow_termination_type == "failure": + raise ApplicationError( + "Deliberately failing workflow with an unfinished handler" + ) await workflow.wait_condition(lambda: False) @workflow.update @@ -5376,23 +5382,44 @@ async def my_signal(self): async def test_unfinished_update_handler_with_workflow_cancellation(client: Client): - await _UnfinishedHandlersWithCancellationTest( - client, "update" - ).test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler() + await _UnfinishedHandlersWithCancellationOrFailureTest( + client, + "update", + "cancellation", + ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() async def test_unfinished_signal_handler_with_workflow_cancellation(client: Client): - await _UnfinishedHandlersWithCancellationTest( - client, "signal" - ).test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler() + await _UnfinishedHandlersWithCancellationOrFailureTest( + client, + "signal", + "cancellation", + ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() + + +async def test_unfinished_update_handler_with_workflow_failure(client: Client): + await _UnfinishedHandlersWithCancellationOrFailureTest( + client, + "update", + "failure", + ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() + + +async def test_unfinished_signal_handler_with_workflow_failure(client: Client): + await _UnfinishedHandlersWithCancellationOrFailureTest( + client, + "signal", + "failure", + ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() @dataclass -class _UnfinishedHandlersWithCancellationTest: +class _UnfinishedHandlersWithCancellationOrFailureTest: client: Client handler_type: Literal["update", "signal"] + workflow_termination_type: Literal["cancellation", "failure"] - async def test_warning_is_issued_when_cancellation_causes_exit_with_unfinished_handler( + async def test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler( self, ): assert await self._run_workflow_and_get_warning() @@ -5402,31 +5429,37 @@ async def _run_workflow_and_get_warning(self) -> bool: update_id = "update-id" task_queue = "tq" - # We require a cancellation request and an update to be delivered in the same WFT. To do - # this we send the start, cancel, and update/signal requests, and then start the worker - # after they've all been accepted by the server. + # We require a startWorkflow, an update, and maybe a cancellation request, to be delivered + # in the same WFT. To do this we start the worker after they've all been accepted by the + # server. handle = await self.client.start_workflow( - UnfinishedHandlersWithCancellationWorkflow.run, + UnfinishedHandlersWithCancellationOrFailureWorkflow.run, + self.workflow_termination_type, id=workflow_id, task_queue=task_queue, ) - await handle.cancel() + if self.workflow_termination_type == "cancellation": + await handle.cancel() if self.handler_type == "update": update_task = asyncio.create_task( handle.execute_update( - UnfinishedHandlersWithCancellationWorkflow.my_update, id=update_id + UnfinishedHandlersWithCancellationOrFailureWorkflow.my_update, + id=update_id, ) ) await assert_eq_eventually( - True, lambda: workflow_update_exists(self.client, workflow_id, update_id) + True, + lambda: workflow_update_exists(self.client, workflow_id, update_id), ) else: - await handle.signal(UnfinishedHandlersWithCancellationWorkflow.my_signal) + await handle.signal( + UnfinishedHandlersWithCancellationOrFailureWorkflow.my_signal + ) async with new_worker( self.client, - UnfinishedHandlersWithCancellationWorkflow, + UnfinishedHandlersWithCancellationOrFailureWorkflow, task_queue=task_queue, ): with pytest.WarningsRecorder() as warnings: From 57952696afc2a5a29501406b5f76fed7c3761992 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 12:01:18 -0400 Subject: [PATCH 40/45] Lint fixes --- tests/worker/test_workflow.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 2ba56ab24..4cd792247 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5370,15 +5370,17 @@ async def run( "Deliberately failing workflow with an unfinished handler" ) await workflow.wait_condition(lambda: False) + raise AssertionError("unreachable") @workflow.update - async def my_update(self) -> str: + async def my_update(self) -> NoReturn: await workflow.wait_condition(lambda: False) - return "update-result" + raise AssertionError("unreachable") @workflow.signal - async def my_signal(self): + async def my_signal(self) -> NoReturn: await workflow.wait_condition(lambda: False) + raise AssertionError("unreachable") async def test_unfinished_update_handler_with_workflow_cancellation(client: Client): @@ -5465,13 +5467,13 @@ async def _run_workflow_and_get_warning(self) -> bool: with pytest.WarningsRecorder() as warnings: if self.handler_type == "update": assert update_task - with pytest.raises(RPCError) as err: + with pytest.raises(RPCError) as update_err: await update_task - assert ( - err.value.status == RPCStatusCode.NOT_FOUND - and "workflow execution already completed" - in str(err.value).lower() - ) + assert ( + update_err.value.status == RPCStatusCode.NOT_FOUND + and "workflow execution already completed" + in str(update_err.value).lower() + ) with pytest.raises(WorkflowFailureError) as err: await handle.result() From f769bc0f2f90ee16b8d1fa79908311cb90d22ab9 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 12:08:03 -0400 Subject: [PATCH 41/45] Skip update tests under Java time-skipping server --- tests/worker/test_workflow.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 4cd792247..de999d560 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5228,7 +5228,11 @@ async def my_signal_WARN_AND_ABANDON(self): await self._do_update_or_signal() -async def test_unfinished_update_handler(client: Client): +async def test_unfinished_update_handler(client: Client, env: WorkflowEnvironment): + if env.supports_time_skipping: + pytest.skip( + "Java test server: https://github.com/temporalio/sdk-java/issues/1903" + ) async with new_worker(client, UnfinishedHandlersWorkflow) as worker: test = _UnfinishedHandlersTest(client, worker, "update") await test.test_wait_all_handlers_finished_and_unfinished_handlers_warning() @@ -5383,7 +5387,13 @@ async def my_signal(self) -> NoReturn: raise AssertionError("unreachable") -async def test_unfinished_update_handler_with_workflow_cancellation(client: Client): +async def test_unfinished_update_handler_with_workflow_cancellation( + client: Client, env: WorkflowEnvironment +): + if env.supports_time_skipping: + pytest.skip( + "Java test server: https://github.com/temporalio/sdk-java/issues/1903" + ) await _UnfinishedHandlersWithCancellationOrFailureTest( client, "update", @@ -5399,7 +5409,13 @@ async def test_unfinished_signal_handler_with_workflow_cancellation(client: Clie ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() -async def test_unfinished_update_handler_with_workflow_failure(client: Client): +async def test_unfinished_update_handler_with_workflow_failure( + client: Client, env: WorkflowEnvironment +): + if env.supports_time_skipping: + pytest.skip( + "Java test server: https://github.com/temporalio/sdk-java/issues/1903" + ) await _UnfinishedHandlersWithCancellationOrFailureTest( client, "update", From cf5b158e61cccfadca135fffad0b95298b5a91bb Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 12:09:20 -0400 Subject: [PATCH 42/45] Remove unused imports --- temporalio/worker/_workflow_instance.py | 2 -- tests/worker/test_workflow.py | 1 - 2 files changed, 3 deletions(-) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index b0cff9ffd..d29b727ea 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -11,13 +11,11 @@ import random import sys import traceback -import typing import warnings from abc import ABC, abstractmethod from contextlib import contextmanager from dataclasses import dataclass from datetime import timedelta -from enum import Enum from typing import ( Any, Awaitable, diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index de999d560..e5be9a3b3 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -42,7 +42,6 @@ from temporalio.api.enums.v1 import EventType from temporalio.api.failure.v1 import Failure from temporalio.api.sdk.v1 import EnhancedStackTrace -from temporalio.api.update.v1 import UpdateRef from temporalio.api.workflowservice.v1 import ( GetWorkflowExecutionHistoryRequest, ResetStickyTaskQueueRequest, From caed0fffceca0066c9ec1d5635fa6607c7e33ca0 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 12:28:30 -0400 Subject: [PATCH 43/45] Try increasing timeout --- tests/worker/test_workflow.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index e5be9a3b3..590d8cd91 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5285,7 +5285,9 @@ async def test_unfinished_handlers_cause_exceptions_in_test_suite(self): ) ) await assert_eq_eventually( - True, partial(self._workflow_task_failed, workflow_id=(await handle).id) + True, + partial(self._workflow_task_failed, workflow_id=(await handle).id), + timeout=timedelta(seconds=20), ) async def _workflow_task_failed(self, workflow_id: str) -> bool: From 97fc45f7282686eef0adfc9ca318dea2ae35d0e9 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Tue, 25 Jun 2024 17:52:09 -0400 Subject: [PATCH 44/45] Remove spurious asyncio.gather calls in test --- tests/worker/test_workflow.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index 590d8cd91..c6e3f3fff 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5338,21 +5338,17 @@ async def _get_workflow_result( if unfinished_policy: handler_name += f"_{unfinished_policy.name}" if self.handler_type == "signal": - await asyncio.gather(handle.signal(handler_name)) + await handle.signal(handler_name) else: if not wait_all_handlers_finished: with pytest.raises(RPCError) as err: - await asyncio.gather( - handle.execute_update(handler_name, id="my-update") - ) + await handle.execute_update(handler_name, id="my-update") assert ( err.value.status == RPCStatusCode.NOT_FOUND and "workflow execution already completed" in str(err.value).lower() ) else: - await asyncio.gather( - handle.execute_update(handler_name, id="my-update") - ) + await handle.execute_update(handler_name, id="my-update") return await handle.result() From b9e07cd7a5e9c073b5759e9555bb98eef6d9a493 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 26 Jun 2024 09:09:21 -0400 Subject: [PATCH 45/45] Skip test due to putative Java test server bug --- tests/worker/test_workflow.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index c6e3f3fff..c63d2de9b 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -5420,7 +5420,13 @@ async def test_unfinished_update_handler_with_workflow_failure( ).test_warning_is_issued_when_cancellation_or_failure_causes_exit_with_unfinished_handler() -async def test_unfinished_signal_handler_with_workflow_failure(client: Client): +async def test_unfinished_signal_handler_with_workflow_failure( + client: Client, env: WorkflowEnvironment +): + if env.supports_time_skipping: + pytest.skip( + "Java test server: https://github.com/temporalio/sdk-java/issues/2127" + ) await _UnfinishedHandlersWithCancellationOrFailureTest( client, "signal",