Null check for TaskContext in AsyncTaskActivity

[leoquijano]

Hello,

I'm doing an upgrade of DurableTask.Core from an older version to v2.16.1. During testing, I noticed that passing a null TaskContext to an instance of AsyncTaskActivity will raise a NullReferenceException, instead of the expected TaskFailureException.

This happens because the exception handler in that method will try to retrieve the ErrorPropagationMode property from the context parameter without doing a null check.

In TaskActivity.cs:

try
{
    result = await ExecuteAsync(context, parameter);
}
catch (Exception e) when (!Utils.IsFatal(e) && !Utils.IsExecutionAborting(e))
{
    string details = null;
    FailureDetails failureDetails = null;
    if (context.ErrorPropagationMode == ErrorPropagationMode.SerializeExceptions)
    {
        details = Utils.SerializeCause(e, DataConverter);
    }
    else
    {
        failureDetails = new FailureDetails(e);
    }

    throw new TaskFailureException(e.Message, e, details)
        .WithFailureDetails(failureDetails);
}

This means that an issue with the passed TaskContext wouldn't have failure details and might fail if the caller's error handling code is expecting TaskFailureException to be raised. Adding a null check in the if statement would fix the issue:

if (context != null && context.ErrorPropagationMode == ErrorPropagationMode.SerializeExceptions)
{
    details = Utils.SerializeCause(e, DataConverter);
}

[cgillum]

Hi @leoquijano - I'm not sure in what cases we'd expect someone to pass a null context value here, but would you be willing to submit a PR with the fix? We'd be happy to review and merge it, giving you credit.

[leoquijano]

Sure! I'll prepare a PR with the fix.

I'm not thinking about an actual use case for passing null (though there might be?), but actually as an error handling issue. For example, if the TaskContext object is generated dynamically (maybe retrieved from a configuration engine), having a TaskFailureException raised instead of a NullReferenceException can help the caller address the issue.

In my case, I found about it due to a unit test that explicitly checks for it.

[leoquijano]

Hi @cgillum, PR #1055 for this change is now ready.