Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public override async Task<object> RunAsync(WorkflowActivityContext context, Pay
this.logger.LogInformation(
"Payment for request ID '{requestId}' could not be processed. Insufficient inventory.",
req.RequestId);
throw new InvalidOperationException();
throw new InvalidOperationException("Not enough inventory!");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Update the statestore with the new amount of the item
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Dapr.Workflow;
using DurableTask.Core.Exceptions;
using WorkflowConsoleApp.Activities;
using WorkflowConsoleApp.Models;

Expand Down Expand Up @@ -44,12 +43,12 @@ await context.CallActivityAsync(
nameof(UpdateInventoryActivity),
new PaymentRequest(RequestId: orderId, order.Name, order.Quantity, order.TotalCost));
}
catch (TaskFailedException)
catch (WorkflowTaskFailedException e)
{
// Let them know their payment was processed
// Let them know their payment processing failed
await context.CallActivityAsync(
nameof(NotifyActivity),
new Notification($"Order {orderId} Failed! You are now getting a refund"));
new Notification($"Order {orderId} Failed! Details: {e.FailureDetails.ErrorMessage}"));
return new OrderResult(Processed: false);
}

Expand Down
34 changes: 30 additions & 4 deletions src/Dapr.Workflow/DaprWorkflowContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ internal DaprWorkflowContext(TaskOrchestrationContext innerContext)

public override Task CallActivityAsync(string name, object? input = null, TaskOptions? options = null)
{
return this.innerContext.CallActivityAsync(name, input, options);
return WrapExceptions(this.innerContext.CallActivityAsync(name, input, options));
}

public override Task<T> CallActivityAsync<T>(string name, object? input = null, TaskOptions? options = null)
{
return this.innerContext.CallActivityAsync<T>(name, input, options);
return WrapExceptions(this.innerContext.CallActivityAsync<T>(name, input, options));
}

public override Task CreateTimer(TimeSpan delay, CancellationToken cancellationToken = default)
Expand Down Expand Up @@ -77,12 +77,12 @@ public override void SetCustomStatus(object? customStatus)

public override Task<TResult> CallChildWorkflowAsync<TResult>(string workflowName, object? input = null, TaskOptions? options = null)
{
return this.innerContext.CallSubOrchestratorAsync<TResult>(workflowName, input, options);
return WrapExceptions(this.innerContext.CallSubOrchestratorAsync<TResult>(workflowName, input, options));
}

public override Task CallChildWorkflowAsync(string workflowName, object? input = null, TaskOptions? options = null)
{
return this.innerContext.CallSubOrchestratorAsync(workflowName, input, options);
return WrapExceptions(this.innerContext.CallSubOrchestratorAsync(workflowName, input, options));
}

public override void ContinueAsNew(object? newInput = null, bool preserveUnprocessedEvents = true)
Expand All @@ -94,5 +94,31 @@ public override Guid NewGuid()
{
return this.innerContext.NewGuid();
}

static async Task WrapExceptions(Task task)
{
try
{
await task;
}
catch (TaskFailedException ex)
{
var details = new WorkflowTaskFailureDetails(ex.FailureDetails);
throw new WorkflowTaskFailedException(ex.Message, details);
}
}

static async Task<TResult> WrapExceptions<TResult>(Task<TResult> task)
{
try
{
return await task;
}
catch (TaskFailedException ex)
{
var details = new WorkflowTaskFailureDetails(ex.FailureDetails);
throw new WorkflowTaskFailedException(ex.Message, details);
}
}
}
}
8 changes: 4 additions & 4 deletions src/Dapr.Workflow/WorkflowContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ public abstract class WorkflowContext
/// <exception cref="InvalidOperationException">
/// Thrown if the calling thread is not the workflow dispatch thread.
/// </exception>
/// <exception cref="TaskFailedException">
/// <exception cref="WorkflowTaskFailedException">
/// The activity failed with an unhandled exception. The details of the failure can be found in the
/// <see cref="TaskFailedException.FailureDetails"/> property.
/// <see cref="WorkflowTaskFailedException.FailureDetails"/> property.
/// </exception>
public virtual Task CallActivityAsync(string name, object? input = null, TaskOptions? options = null)
{
Expand Down Expand Up @@ -252,9 +252,9 @@ public virtual Task CreateTimer(TimeSpan delay, CancellationToken cancellationTo
/// <exception cref="InvalidOperationException">
/// Thrown if the calling thread is not the workflow dispatch thread.
/// </exception>
/// <exception cref="TaskFailedException">
/// <exception cref="WorkflowTaskFailedException">
/// The child workflow failed with an unhandled exception. The details of the failure can be found in the
/// <see cref="TaskFailedException.FailureDetails"/> property.
/// <see cref="WorkflowTaskFailedException.FailureDetails"/> property.
/// </exception>
public virtual Task CallChildWorkflowAsync(string workflowName, object? input = null, TaskOptions? options = null)
{
Expand Down
39 changes: 39 additions & 0 deletions src/Dapr.Workflow/WorkflowTaskFailedException.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// ------------------------------------------------------------------------
// Copyright 2023 The Dapr Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
// http://www.apache.org/licenses/LICENSE-2.0
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// ------------------------------------------------------------------------

namespace Dapr.Workflow
{
using System;

/// <summary>
/// Exception type for Dapr Workflow task failures.
/// </summary>
public class WorkflowTaskFailedException : Exception
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
/// <summary>
/// Initializes a new instance of the <see cref="WorkflowTaskFailedException"/> class.
/// </summary>
/// <param name="message">The exception message.</param>
/// <param name="failureDetails">Details about the failure.</param>
public WorkflowTaskFailedException(string message, WorkflowTaskFailureDetails failureDetails)
: base(message)
{
this.FailureDetails = failureDetails ?? throw new ArgumentNullException(nameof(failureDetails));
}

/// <summary>
/// Gets more information about the underlying workflow task failure.
/// </summary>
public WorkflowTaskFailureDetails FailureDetails { get; }
}
}