Null check for TaskContext in AsyncTaskActivity
Closed this issue · 3 comments
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);
}
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.
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.