Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware for exceptions handling #1039

Open
saguiitay opened this issue Feb 11, 2024 · 4 comments
Open

Middleware for exceptions handling #1039

saguiitay opened this issue Feb 11, 2024 · 4 comments

Comments

@saguiitay
Copy link
Contributor

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
Copy link
Collaborator

cgillum commented Feb 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

cgillum commented Mar 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants