[Workflow] Add WorkflowTaskFailedException for workflow error handling#1086
Conversation
Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Codecov Report
@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
- Coverage 68.09% 68.07% -0.02%
==========================================
Files 169 169
Lines 5604 5604
Branches 592 592
==========================================
- Hits 3816 3815 -1
- Misses 1648 1649 +1
Partials 140 140
Flags with carried forward coverage won't be shown. Click here to find out more. |
| /// <summary> | ||
| /// Exception type for Dapr Workflow task failures. | ||
| /// </summary> | ||
| public class WorkflowTaskFailedException : Exception |
There was a problem hiding this comment.
Based on the issue, it seems like this should be inheriting from a type that isn't raw Exception. They were looking for a pretty specific exception type and this also won't be caught.
Any reason it's a base Exception instead of what the user was looking for?
Also, this will be a breaking change so we'll have to flag it as such.
There was a problem hiding this comment.
Yes, this is intentionally a breaking change. The TaskFailedException that Marc was trying to catch is actually not something we want to expose publicly since it's defined by the Durable Task Framework, and we're trying to hide those implementation details from the programming model. We solve the problem of leaking Durable types by creating our own exception type (WorkflowTaskFailedException) and asking users to catch that one instead.
| "Payment for request ID '{requestId}' could not be processed. Insufficient inventory.", | ||
| req.RequestId); | ||
| throw new InvalidOperationException(); | ||
| throw new InvalidOperationException("Not enough inventory!"); |
There was a problem hiding this comment.
Not a blocker for this review, but can we include the item that we are trying to purchase here as well as the number of items still available of that type for purchase?
There was a problem hiding this comment.
Agreed, that would have been better. It's actually really difficult to hit this case today since the ReserveInventoryActivity will do this check already before we get here, so I don't think users playing with the sample will ever hit this, but I'll update it if there are other blocking things that need to be done for this PR.
Signed-off-by: Chris Gillum <cgillum@microsoft.com> Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
3 participants
Description
This change adds a new
WorkflowTaskFailedExceptiontype for handling activity and child-workflow exceptions from inside a workflow. It's meant to replace the usage ofTaskFailedExceptiontype defined in the Durable Task SDK, which we're trying to hide.This PR also fixes a bug in the console app sample for workflows, plus cleaning up a few things.
This is a breaking change, but one that we were planning to make anyways since v1.10.
Issue reference
Fixes #1081
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
FYI @RyanLettieri @DeepanshuA @nyemade-uversky @marcduiker