Ability for certain task failure types to fail workflow #205

cretz · 2024-03-18T17:23:49Z

What was changed

Added TemporalWorkerOptions.WorkflowFailureExceptionTypes to allow configuring failure exception types at a worker level
Added WorkflowAttribute.FailureExceptionTypes to allow configuring failure exception types at a workflow level
Update core to get Enable failing WF on nondeterminism errors sdk-core#702
Abstracted InvalidWorkflowOperationException into separate exceptions for InvalidWorkflowSchedulerException and WorkflowNondeterminismException
- This exception used to be used for both, but it's abstracted now so the latter can be referenced as a failure type
Updated bridge code to include worker-level and workflow-type-level non-determinism-as-fail based on the presence of WorkflowNondeterminismException (or super type) in worker/workflow options
Documented in README, API doc, and marked experimental
Several tests for several scenarios

Checklist

Closes [Feature Request] Ability for certain task failure types to fail workflow #166

cretz · 2024-03-18T17:30:18Z

src/Temporalio/Exceptions/WorkflowNondeterminismException.cs

+    /// this exception can be used as a failure exception type to have non-deterministic exceptions
+    /// fail workflows.
+    /// </remarks>
+    public class WorkflowNondeterminismException : InvalidWorkflowOperationException


So Java capitalizes the D here but core does not, open to suggestions

Sushisource · 2024-03-18T17:33:10Z

src/Temporalio/Bridge/include/temporal-sdk-bridge.h

+  bool nondeterminism_as_workflow_fail;
+  /**
+   * This is expected to be a newline-delimited list
+   */
+  struct ByteArrayRef nondeterminism_as_workflow_fail_for_types;


Why have these specially for nondeterminism rather than passing through the types and converting that to the Rust types?

If/when we add more types there will need to be two fields for every future type rather than just adding a branch to the converter

Because this is a private FFI boundary and I only expose the most I need. I am trying to avoid overcomplicating the FFI code until there is a second type. I would not be surprised if that enum stays as only one item forever, heh, but it's much easier for Rust to build those abstractions than C.

Sure, but, you've got to do the conversion either way. It's either the same amount of complicated, or if we ever add anything else, less complicated. You can just pass through an an enum or something, super simple, and less work to extend later.

It's more complicated to do C arrays (not "super simple") and I have been lucky to avoid them so far because I can make expectations about how I can split single-value fields. But it sounds like my luck is running out, ug.

I just added string arrays in this commit and removed the newline restriction

Sushisource · 2024-03-18T17:35:56Z

src/Temporalio/Bridge/src/worker.rs

+                opt.nondeterminism_as_workflow_fail_for_types
+                    .to_option_str()
+                    .map(|s| {
+                        s.split('\n')


You could make the to map helper for this generic and use it here.

I'm still not a fan of parsing on newlines like this, but it's not hugely important to change.

I already have a map helper for metadata (and it's newline delimited too), but I admit being lazy, because getting collections of strings right in a non-annoying way over FFI boundary requires basically building your own structures (or saying something like \0 delimited) and I was hoping to avoid it.

I just added string arrays in this commit and removed the newline restriction

Sushisource · 2024-03-18T17:37:45Z

src/Temporalio/Worker/TemporalWorkerOptions.cs

+        /// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure
+        /// all others. But this default may change in the future.


Suggested change

/// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure

/// all others. But this default may change in the future.

/// <see cref="Exceptions.FailureException" />. Other exception types will cause either cancellation or suspension via task failure. This default may change in the future.

This isn't necessarily true with how the code is today. I trap cancellation errors and return workflow fail even when cancellation was not requested because of how common they appear and that it's unfair to have already-started timer cancellation be a wf fail but not-yet-started timer cancellation be a task fail.

Sushisource · 2024-03-18T17:39:02Z

src/Temporalio/Workflows/WorkflowAttribute.cs

+        /// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure
+        /// all others. But this default may change in the future.


Suggested change

/// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure

/// all others. But this default may change in the future.

/// <see cref="Exceptions.FailureException" />. Other exception types will cause either cancellation or suspension via task failure. This default may change in the future.

See other comment on similar change

Sushisource · 2024-03-18T17:39:33Z

src/Temporalio/Workflows/WorkflowDefinition.cs

+            if (name != null && name.Contains('\n'))
+            {
+                errs.Add("Workflow name cannot have a newline");
+            }


This is a good example why the newline thing scares me.

I mean, it's dumb to have a newline in a workflow name anyway, but, still.

It already occurs with metadata today, but I am preventing the newline explicitly. I don't think this use case is worth me building my own C string vec representation. But I can if I must.

Well does that really mean we need to build our own? I've got to imagine this comes up fairly often...

And yeah, I know it's happening with metadata but the fact that we'll sometimes have to deal with edge cases like this as we add new things says to me it probably is worth a little bit of effort to just deal with that problem for good.

In C what you have to do is accept a pointer to the first element and then a length. And each element needs to know its size. And with ownership rules over FFI, it means each element needs to remain owned on the lang side. So it's a pointer to an owned array of pointers to owned strings. But this is totally doable, I've just avoided it because I've never needed it and it's ugly/dangerous. Didn't seem worth it just to keep newlines in workflow types, but I will try to add it now.

I just added string arrays in this commit and removed the newline restriction

Sushisource

Cool! Yeah, that looked pretty easy and worth it.

Allow customization of task failure exception

9688ae9

Fixes temporalio#166

cretz requested a review from a team March 18, 2024 17:23

cretz commented Mar 18, 2024

View reviewed changes

Sushisource reviewed Mar 18, 2024

View reviewed changes

Support string arrays over FFI boundary

07524da

Sushisource approved these changes Mar 20, 2024

View reviewed changes

cretz merged commit 67a52ff into temporalio:main Mar 21, 2024
6 checks passed

cretz deleted the workflow-failure-exception-types branch March 21, 2024 12:55

cretz mentioned this pull request Apr 25, 2024

Ability for certain task failure types to fail workflow temporalio/sdk-python#516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability for certain task failure types to fail workflow #205

Ability for certain task failure types to fail workflow #205

cretz commented Mar 18, 2024

cretz Mar 18, 2024

Sushisource Mar 18, 2024

cretz Mar 18, 2024

Sushisource Mar 18, 2024

cretz Mar 18, 2024 •

edited

Loading

cretz Mar 20, 2024

Sushisource Mar 18, 2024

cretz Mar 18, 2024

cretz Mar 20, 2024

Sushisource Mar 18, 2024

cretz Mar 18, 2024 •

edited

Loading

Sushisource Mar 18, 2024

cretz Mar 20, 2024

Sushisource Mar 18, 2024

cretz Mar 18, 2024

Sushisource Mar 19, 2024

cretz Mar 20, 2024 •

edited

Loading

cretz Mar 20, 2024

Sushisource left a comment

		/// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure
		/// all others. But this default may change in the future.

Ability for certain task failure types to fail workflow #205

Ability for certain task failure types to fail workflow #205

Conversation

cretz commented Mar 18, 2024

What was changed

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource left a comment

Choose a reason for hiding this comment

cretz Mar 18, 2024 •

edited

Loading

cretz Mar 18, 2024 •

edited

Loading

cretz Mar 20, 2024 •

edited

Loading