Azure / durabletask

Durable Task Framework allows users to write long running persistent workflows in C# using the async/await capabilities.
Apache License 2.0
1.47k stars 287 forks source link

Middleware for exceptions handling #1039

Open saguiitay opened 4 months ago

saguiitay commented 4 months ago

I've created a middleware to centrally handle various types of exceptions. Before my change, I had a try-catch block in my TaskActivity.Execute() method:

protected override string Execute(TaskContext context, string input)
{
    try
    {
        ...
    }
    catch (BadExceptionType e)
    {
        throw new GoodExceptionType();
    }
}

This worked fine regarding retries when an exception is thrown. However, when I moved this logic to a middleware, RetryOptions were disregarded, and the same activity was retried over-and-over indefinitely:

public override async Task Dispatch(DispatchMiddlewareContext context, Func<Task> next)
{
    try
    {
        await next();
    }
    catch (BadExceptionType e)
    {
        throw new GoodExceptionType();
    }
}

Is there some guidance on handling exceptions in middleware? Is there a correct way to handle this scenario? I've tried simulating the behavior of the dispatcher, and managed to get this to work, but that feels incorrect:

public override async Task Dispatch(DispatchMiddlewareContext context, Func<Task> next)
{
    try
    {
        await next();
    }
    catch (Exception e)
    {
        var tfe = (TaskFailureException)e;

        var failureEvent = new TaskFailedEvent(-1, taskScheduledEvent.EventId, tfe!.InnerException!.Message, null, new FailureDetails(tfe!.InnerException));
        var result = new ActivityExecutionResult { ResponseEvent = failureEvent };
        context.SetProperty(result);
    }
}

@cgillum @davidmrdavid

cgillum commented 4 months ago

The current design of the middleware pipeline doesn't really help with exception handling, as you've noticed. It's very low-level and assumes knowledge of how the dispatcher works. I think we'd need some enhancements or redesigns to create a more developer-friendly experience for middleware.

saguiitay commented 4 months ago

Thanks for the update. Is my suggested solution technically correct? Should I go that way until the team come up with a better design? Is that in the near-future roadmap?

saguiitay commented 3 months ago

Gentle ping regarding these questions:

Is my suggested solution technically correct? Should I go that way until the team come up with a better design?

cgillum commented 3 months ago

Is my suggested solution technically correct?

Yes, this is correct. As a reference, you can see what we're doing with this middleware in the Durable Functions extension here.

Should I go that way until the team come up with a better design?

We don't have any alternatives, and we ourselves rely on this pretty heavily (and thus won't break it) so I'd say it's safe to go this route for now.

Is that in the near-future roadmap?

This is not on our roadmap, unfortunately. However, we'd be willing to accept it as a contribution since it seems like a generally useful feature.