New API to wait for handler executions to complete and warnings on unfinished handler executions #556

dandavison · 2024-06-19T19:49:53Z

Closes #538

Implementation ready for review; names are still under discussion.

What was changed

Added a new API to wait for update and signal handler executions to complete. Provisionally named workflow.all_handlers_finished().
Issue warnings on workflow exit if any handlers are unfinished.
Allow warnings to be disabled on a per-handler basis via a signal/update decorator parameter.

Why?

Workflows exiting while handler executions are in progress is an important category of run-time workflow incorrectness.

dandavison · 2024-06-19T20:47:25Z

temporalio/workflow.py

-    return with_name(fn.__name__, fn)
+        return partial(decorator, name, unfinished_handlers_policy)
+    else:
+        return decorator(fn.__name__, unfinished_handlers_policy, fn)


These changes make the decorator behave consistently: permitting e.g. @update() with the same meaning as @update, and allowing the default value of the handler policy to be passed explicitly as a param if the user desires. That makes sense, especially now that we're adding a 3rd parameter to the decorators which is unrelated to name and dynamic. TODO: I'll need to update the query decorator also for consistency.

cretz

Looks good. Most of my comments are only on little things.

cretz · 2024-06-20T13:54:41Z

temporalio/worker/_workflow_instance.py

+        # 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[


Unsure if dict is supported in 3.8, we have used Dict (from typing) to be sure to have compatibility

Thanks! I was trying to remember to use the 3.8-compatible upper-cased names but it's easy to forget.

cretz · 2024-06-20T13:58:44Z

temporalio/worker/_workflow_instance.py

@@ -490,6 +539,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)


I think this should move to just above the try for clarity (even though it doesn't matter much in practice, was just a bit confusing to have to hunt down the cases when something undone in a finally was not done)

As things stand, it has to be here, since defn is bound inside the try. I agree that the try should be scoped more tightly around the code which is actually raising the exceptions it's catching, but I think I'll leave that change out of this PR.

temporalio/worker/_workflow_instance.py

temporalio/workflow.py

tests/worker/test_workflow.py

dandavison

Thanks for the review @cretz, I've taken almost all the suggestions you made.

dandavison · 2024-06-20T20:22:50Z

temporalio/worker/_workflow_instance.py

+        # 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[


Thanks! I was trying to remember to use the 3.8-compatible upper-cased names but it's easy to forget.

temporalio/worker/_workflow_instance.py

dandavison · 2024-06-20T22:22:25Z

temporalio/workflow.py

+    # an error rather than the update result.
+    ABANDON = 1
+    # Issue a warning in addition to abandoning.
+    WARN_AND_ABANDON = 2


Yep, I had it that way and then changed it to make the comments read better. Changed it back.

dandavison · 2024-06-20T22:23:07Z

temporalio/workflow.py

@@ -173,6 +173,20 @@ def run(fn: CallableAsyncType) -> CallableAsyncType:
    return fn  # type: ignore[return-value]


+class UnfinishedHandlersPolicy(IntEnum):


temporalio/workflow.py

pyproject.toml

temporalio/workflow.py

temporalio/worker/_workflow_instance.py

tests/worker/test_workflow.py

cretz

This LGTM. Need to disable tests for time-skipping test server and may need to remove an unused import, but overall LGTM.

dandavison · 2024-06-25T15:04:10Z

This PR was missing one thing -- it didn't address cancellation. Discussed with @cretz in a different channel and we agreed that it was a bug that the CancelWorkflowExecution command was not being defined to be a workflow "completion" by the SDK. Recent commits fix that bug and add the test coverage that this PR had been missing regarding cancellation.

dandavison requested a review from a team as a code owner June 19, 2024 19:49

dandavison force-pushed the sdk-2228-unfinished-handlers branch from bea62d5 to 6e9cf4c Compare June 19, 2024 19:54

dandavison commented Jun 19, 2024

View reviewed changes

cretz reviewed Jun 20, 2024

View reviewed changes

dandavison commented Jun 21, 2024

View reviewed changes

cretz reviewed Jun 21, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

dandavison force-pushed the sdk-2228-unfinished-handlers branch from 7e1fc7b to 62d7c03 Compare June 21, 2024 18:17

cretz reviewed Jun 21, 2024

View reviewed changes

temporalio/workflow.py Outdated Show resolved Hide resolved

dandavison force-pushed the sdk-2228-unfinished-handlers branch 2 times, most recently from 94bf238 to 9346ac1 Compare June 21, 2024 21:07

cretz reviewed Jun 24, 2024

View reviewed changes

temporalio/worker/_workflow_instance.py Outdated Show resolved Hide resolved

cretz reviewed Jun 24, 2024

View reviewed changes

tests/worker/test_workflow.py Show resolved Hide resolved

cretz reviewed Jun 24, 2024

View reviewed changes

tests/worker/test_workflow.py Show resolved Hide resolved

cretz approved these changes Jun 24, 2024

View reviewed changes

dandavison added 15 commits June 25, 2024 11:06

Add failing test

ed5bd1a

Refactor test

f675216

Implement warning on unfinished signals and updates

c2a1961

Implement all_handlers_finished

8945ea6

Test waiting for all_handlers_finished

8a5f432

Run in same worker

8a1c7e6

Refactor

8eef8a6

Refactor test

ee4fe48

Store policy with in-progress job

4c6bb27

Add new parameter to signal and update decorators

916cd72

Store (job, defn)

9c34eae

Format warning messages

6feed46

Improve test

e024ef2

Clean up warning implementation

2f96acd

s/finished/complete/

43387e4

dandavison added 3 commits June 25, 2024 11:06

Use default timeout and interval

fcabe21

Clean up imports

ea63920

Edit warning messages

7063c17

dandavison force-pushed the sdk-2228-unfinished-handlers branch from 2e39d45 to f59bcac Compare June 25, 2024 15:13

dandavison added 3 commits June 25, 2024 11:25

Refactor test

8162e31

Test cancellation

15ed853

Bug fix: include cancellation command in set of "completion" commands

3f0b11c

dandavison force-pushed the sdk-2228-unfinished-handlers branch from f59bcac to 3f0b11c Compare June 25, 2024 15:26

cretz approved these changes Jun 25, 2024

View reviewed changes

dandavison force-pushed the sdk-2228-unfinished-handlers branch from 42e3fc6 to d6ca246 Compare June 25, 2024 15:50

dandavison added 6 commits June 25, 2024 11:50

Add coverage for workflow failure

d6ca246

Lint fixes

5795269

Skip update tests under Java time-skipping server

f769bc0

Remove unused imports

cf5b158

Try increasing timeout

caed0ff

Remove spurious asyncio.gather calls in test

97fc45f

dandavison force-pushed the sdk-2228-unfinished-handlers branch from 54d9ac9 to 6526fb1 Compare June 26, 2024 09:35

dandavison mentioned this pull request Jun 26, 2024

Test server sometimes fails to include signal in first WFT temporalio/sdk-java#2127

Open

dandavison force-pushed the sdk-2228-unfinished-handlers branch 2 times, most recently from d64fa77 to b9e07cd Compare June 26, 2024 13:32

Skip test due to putative Java test server bug

b9e07cd

dandavison merged commit 2c1ac54 into main Jun 26, 2024
12 checks passed

dandavison deleted the sdk-2228-unfinished-handlers branch June 26, 2024 17:57

cretz mentioned this pull request Jul 2, 2024

Warn on unfinished workflow handlers temporalio/sdk-dotnet#294

Merged

dandavison mentioned this pull request Jul 17, 2024

Unfinished handlers: continue-as-new and dynamic handlers #584

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API to wait for handler executions to complete and warnings on unfinished handler executions #556

New API to wait for handler executions to complete and warnings on unfinished handler executions #556

dandavison commented Jun 19, 2024 •

edited

Loading

dandavison Jun 19, 2024 •

edited

Loading

cretz left a comment

cretz Jun 20, 2024

dandavison Jun 20, 2024

cretz Jun 20, 2024

dandavison Jun 25, 2024

dandavison left a comment

dandavison Jun 20, 2024

dandavison Jun 20, 2024

dandavison Jun 20, 2024

cretz left a comment

dandavison commented Jun 25, 2024

		@@ -173,6 +173,20 @@ def run(fn: CallableAsyncType) -> CallableAsyncType:
		return fn # type: ignore[return-value]


		class UnfinishedHandlersPolicy(IntEnum):

New API to wait for handler executions to complete and warnings on unfinished handler executions #556

New API to wait for handler executions to complete and warnings on unfinished handler executions #556

Conversation

dandavison commented Jun 19, 2024 • edited Loading

What was changed

Why?

dandavison Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

cretz Jun 20, 2024

Choose a reason for hiding this comment

dandavison Jun 20, 2024

Choose a reason for hiding this comment

cretz Jun 20, 2024

Choose a reason for hiding this comment

dandavison Jun 25, 2024

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

dandavison Jun 20, 2024

Choose a reason for hiding this comment

dandavison Jun 20, 2024

Choose a reason for hiding this comment

dandavison Jun 20, 2024

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

dandavison commented Jun 25, 2024

dandavison commented Jun 19, 2024 •

edited

Loading

dandavison Jun 19, 2024 •

edited

Loading