Activity Failures should return activity's exception, not a TaskFailedException

[TsuyoshiUshio]

I tried a feature of Custom RetryOption. However, I found an issue.

1. Exception Type is NOT FunctionFailedException

I write code like following.

            var option = new RetryOptions(System.TimeSpan.FromSeconds(5), 3);
                option.Handle = (e) => {                
                    if (!context.IsReplaying)
                    {
                        log.Info("******* Call back called");
                        log.Info(e.ToString());
                        log.Info("*****");
                    }
                    return true; };
                outputs.Add(await context.CallActivityWithRetryAsync<string>("DurableSpike_RetryIt", option, "RetryIt"));
                     :

        [FunctionName("DurableSpike_RetryIt")]
        public static async Task<string> RetryItAsync([ActivityTrigger] string name, TraceWriter log)
        {
            log.Info("Activity RetryItAsync has been called");
            throw new System.Exception("This is execption from Activity");
        }

I expect the lambda's parameter for the RetryOption.Handle should be FunctionFailedException. However it is TaskFailedException. I use Durable Functions v1.4.1

[17/05/2018 05:28:19] ******* Call back called
[17/05/2018 05:28:19] DurableTask.Core.Exceptions.TaskFailedException: Exception while executing function: DurableSpike_RetryIt
[17/05/2018 05:28:19]    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
[17/05/2018 05:28:19]    at DurableTask.Core.TaskOrchestrationContext.<ScheduleTaskInternal>d__14.MoveNext()
[17/05/2018 05:28:19] --- End of stack trace from previous location where exception was thrown ---
[17/05/2018 05:28:19]    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
[17/05/2018 05:28:19]    at DurableTask.Core.TaskOrchestrationContext.<ScheduleTaskToWorker>d__13`1.MoveNext()
[17/05/2018 05:28:19] --- End of stack trace from previous location where exception was thrown ---
[17/05/2018 05:28:19]    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
[17/05/2018 05:28:19]    at DurableTask.Core.TaskOrchestrationContext.<ScheduleTask>d__12`1.MoveNext()
[17/05/2018 05:28:19] --- End of stack trace from previous location where exception was thrown ---
[17/05/2018 05:28:19]    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[17/05/2018 05:28:19]    at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
[17/05/2018 05:28:19]    at DurableTask.Core.RetryInterceptor`1.<Invoke>d__4.MoveNext()
[17/05/2018 05:28:19] *****

To Reproduce, you can use this code.

using System.Collections.Generic;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.Azure.WebJobs;
using Microsoft.Azure.WebJobs.Extensions.Http;
using Microsoft.Azure.WebJobs.Host;

namespace DurableFunctionSpike
{
    public static class DurableSpike
    {
        [FunctionName("DurableSpike")]
        public static async Task<List<string>> RunOrchestrator(
            [OrchestrationTrigger] DurableOrchestrationContext context, TraceWriter log)
        {
            var outputs = new List<string>();

            // Replace "hello" with the name of your Durable Activity Function.
            outputs.Add(await context.CallActivityAsync<string>("DurableSpike_Hello", "Tokyo"));
            outputs.Add(await context.CallActivityAsync<string>("DurableSpike_Hello", "Seattle"));

            outputs.Add(await context.CallActivityAsync<string>("DurableSpike_Hello", "London"));


            var option = new RetryOptions(System.TimeSpan.FromSeconds(5), 3);
                option.Handle = (e) => {
                    if (!context.IsReplaying)
                    {
                        log.Info("******* Call back called");
                        log.Info(e.ToString());
                        log.Info("*****");
                    }
                    return true; };

            outputs.Add(await context.CallActivityWithRetryAsync<string>("DurableSpike_RetryIt", option, "RetryIt"));
            // returns ["Hello Tokyo!", "Hello Seattle!", "Hello London!"]
            return outputs;
        }

        [FunctionName("DurableSpike_Hello")]
        public static string SayHello([ActivityTrigger] string name, TraceWriter log)
        {
            log.Info($"Saying hello to {name}.");
            return $"Hello {name}!";
        }
        
        [FunctionName("DurableSpike_RetryIt")]
        public static async Task<string> RetryItAsync([ActivityTrigger] string name, TraceWriter log)
        {
            log.Info("Activity RetryItAsync has been called");
            throw new System.Exception("This is execption from Activity");
            //while(true)
            //{
            //    await Task.Delay(10 * 1000);
            //}
            //return "Never finish";

        }

        [FunctionName("DurableSpike_HttpStart")]
        public static async Task<HttpResponseMessage> HttpStart(
            [HttpTrigger(AuthorizationLevel.Anonymous, "get", "post")]HttpRequestMessage req,
            [OrchestrationClient]DurableOrchestrationClient starter,
            TraceWriter log)
        {
            // Function input comes from the request content.
            string instanceId = await starter.StartNewAsync("DurableSpike", null);

            log.Info($"Started orchestration with ID = '{instanceId}'.");

            return starter.CreateCheckStatusResponse(req, instanceId);
        }
    }
}

[shibayan]

@TsuyoshiUshio This will be the intended behavior. RetryOptions#Handle is only a wrapper around DurableTask.Core.
(FunctionFailedException is just a change of the contents of TaskFailedException.)

However, I feel some discomfort. I'd like to receive exceptions that actually occurred rather than DurableTask exception types.

Example

var option = new RetryOptions(System.TimeSpan.FromSeconds(5), 3)
{
    // Dislike it
    // ex is TaskFailedException or SubOrchestrationFailedException
    //Handle = (ex.InnerException is HttpRequestException || ex.InnerException is SocketException)

    // Like it
    Handle = (ex is HttpRequestException || ex is SocketException)
};

Since it is a retrying process, I do not feel the necessity of wrapping with another Exception.
Thanks.

[TsuyoshiUshio]

Hi @shibayan
Thank you for the comment. I can't remember why I thought something like this. Maybe documentation at that time? Anyway, we might be able to close this issue. :) Also your recommendation is totally make sense. Currently, RetryIntercepter is executed on the OrchestrationContext for example. Activity and Orchestration Dispatcher throws TaskFailedException. So the RetryIntercepter accept the TaskFailedException. We might can change that part, however, it can be an breaking change. What do you think @cgillum ?

[shibayan]

In my opinion, it is possible to support by adding a custom implementation of RetryOptions#Handle without touching the DurableTask.Core.

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/RetryOptions.cs

Lines 95 to 99 in 11463dd

	public Func<Exception, bool> Handle
	{
	get { return this.retryOptions.Handle; }
	set { this.retryOptions.Handle = value; }
	}

I want to make an optimal retry for each exception :)

[TsuyoshiUshio]

It much better than I wrote! Thank you. However, it still introduce the behavior change for the Durable Functions's side. We have a good chance to have a breaking change so, Let's wait for the Chris's reaction. :)

[cgillum]

Yes, this would be a breaking change. That's okay though since we are starting to plan for the 2.0 release, which is a breaking change release. We can include this change as part of 2.0.

[mcgear]

Hello, this is a little off topic, but not entirely... I am wondering why only support exceptions in terms of retry? It would be nice to have a handle for an exception and a handle for the actual return type... Say the Activity actually wants to intelligently handle exceptions and return some type of response and based on that response execute or don't execute retry?

[cgillum]

We had a similar request for something like this recently. Take a look at this issue and feel free to add comments.

[ConnorMcMahon]

This should be tracked alongside of #313, wrapped behind a feature flag.