Improve exception handling in .NET isolated #3034
Conversation
- Serialize exception details in worker middleware - Attempt to deserialize in host middleware - Ensure inner exceptions + complex messages are preserved
andystaples
changed the title
|
|
||
| public override string? StackTrace => this.FromException.StackTrace; | ||
|
|
||
| private static TaskFailureDetails? ExceptionToTaskFailureDetailsRecursive(Exception? fromException) |
There was a problem hiding this comment.
Do we have a shared version of this we can use? Or is it in some inaccessible place, so we have to copy it? I'm a tiny bit worried about keeping multiple copies up-to-date if we decide to add richer information to TaskFailureDetails (which is very likely).
There was a problem hiding this comment.
Moved to a new TaskFailureDetailsConverter
| } | ||
| catch (Exception) | ||
| { | ||
| // Apparently the exception message was not serialized by the worker middleware, continue |
There was a problem hiding this comment.
Do we want to log a message saying what's in this comment? Will this be useful for debugging issues in the future?
There was a problem hiding this comment.
Done - the actual warning is logged using TraceHelper.ExtensionWarningEvent in CallActivityAsync based on whether the GetFailureDetails call reports successful deserialization of the new message.
If there is a better way to log this, please, let me know
There was a problem hiding this comment.
Oh also, a consideration: GetFailureDetails is used for all languages that use OutOfProcMiddleware, so I believe Java/others will now log this warning on every activity failure, as the worker extensions for these languages do not know to serialize the exception details in this way.
We may eventually want to update the other extensions in a similar way - happy to remove/comment this warning until we expect all languages to support exception serialization, or I can try to check the language and only warn for dotnet, or leave the warning in as-is, if we think that the warning will not be distracting for other languages.
There was a problem hiding this comment.
Update - added a check for dotnet isolated
There was a problem hiding this comment.
as the worker extensions for these languages do not know to serialize the exception details in this way.
Can you clarify what you mean here?
- Add tests for entity error handling - Revert to synchronous worker middleware - Log a warning if exception message not serialized
|
|
||
| internal class DurableSerializationException : Exception | ||
| { | ||
| private Exception FromException; |
There was a problem hiding this comment.
Fields should be lowercased and, whenever possible, should be readonly.
| private Exception FromException; | |
| private readonly Exception fromException; |
| [DurableClient] DurableTaskClient client, | ||
| FunctionContext executionContext) | ||
| { | ||
| ILogger logger = executionContext.GetLogger("RethrowActivityException_HttpStart"); |
There was a problem hiding this comment.
Should this be:
| ILogger logger = executionContext.GetLogger("RethrowActivityException_HttpStart"); | |
| ILogger logger = executionContext.GetLogger("CatchHttpStart"); |
|
|
||
| public static class ActivityErrorHandling | ||
| { | ||
| private static ConcurrentDictionary<string, int> retryCount = new ConcurrentDictionary<string, int>(); |
There was a problem hiding this comment.
nit: consider naming this globalRetryCount to make it clear that it's a global (static) value.
| for (int i = 0; i < 5; i++) | ||
| { | ||
| Thread.Sleep(1000); | ||
| } |
There was a problem hiding this comment.
What's the purpose of having a sleep in a for-loop vs. just doing Thread.Sleep(5000)?
Improves exception handling in .NET isolated by serializing the exception details in the worker middleware, then deserializing them back in host middleware.
This may introduce breaking changes to the way that the Functions Host logs exceptions from Durable activities.
Also adds testing for error handling, activity retry, and activity timeout scenarios in dotnet-isolated
Resolves #2711
Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.