Azure-functions-durable-extension: Failed to deserialize SqlException from TaskActivity

[marcin-synak]

Error similar to the one fixed in #459 is still happening in some cases in v1.8.0.

I have a much simpler case where simple orchestrator calls an activity that executes some SQL query. The query fails and activity throws SqlException. It seems that this kind of exception is especially nasty to serialize:

Microsoft.Azure.WebJobs.FunctionFailedException: The activity function 'GetViewers' failed: "Failed to deserialize exception from TaskActivity: {"$type":"System.Data.SqlClient.SqlException, System.Data.SqlClient","ClassName":"System.Data.SqlClient.SqlException","Message":"Invalid object name 'ActiveViewer_1_19'."

The whole stack trace is very long, but I think this is an interesting part:

System.InvalidCastException: Unable to cast object of type 'Newtonsoft.Json.Linq.JValue' to type 'System.Guid'. at System.Data.SqlClient.SqlException..ctor(SerializationInfo si, StreamingContext sc) at lambda_method(Closure , Object[] ) at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateISerializable(JsonReader reader, JsonISerializableContract contract, JsonProperty member, String id)

There is indeed a Guid field in SqlException and it seems it causes the serialization issue:

"ClientConnectionId":"b8c66db2-dfcc-47f1-9f33-f3863f07b387"

This should be very easy to reproduce. Just make an activity that executes wrong SQL.

[tompostler]

I believe this is because Azure Functions (and Durable Tasks) run with Newtonsoft v9. Newtonsoft v10 has a fix to load assemblies by partial name match (lines 80-81: https://github.com/JamesNK/Newtonsoft.Json/commit/4605894b6b3c34859baa0561135e75b5ff8895c5#diff-0bb45c7f4b1341bf2e93b461f66bd0f5).

However, I'm struggling to confirm that a later version of Newtonsoft would fix this issue. No level of static or runtime binding redirects that I can define appear to affect the v9 Newtonsoft usage.

[ConnorMcMahon]

We can potentially fix this for the v2 release of Durable Functions extension. We should at least be able to fix this for Functions V2. Functions V1 extensions have more limitations about what dependencies they can use due to the lack of separation of app domains in the Functions V1 runtime.

[b10-dslappendel]

I just hit this problem as well. However the message in my case is the following:

Unable to find a constructor to use for type System.Data.SqlClient.SqlException. A class should either have a default constructor, one constructor with arguments or a constructor marked with the JsonConstructor attribute. Path 'ClassName', line 1, position 81.

I am using .NET Core 3.1.
Is there something that I can use to work around this issue?

[ConnorMcMahon]

I would try injecting an IErrorSerializerSettingsFactory (introduced in v2.1) with custom JsonConverter objects on it to get around this issue. We will be adding official documentation for how to do this soon (#1208).

[beerai]

I'm seeing the same error message as @b10-dslappendel recently.

@ConnorMcMahon could you elaborate on your workaround?

[ConnorMcMahon]

This stack overflow answer shows how to use dependency injection to use customer serialization settings for messages. You can follow a nearly identical solution, but with IErrorSerializerSettingsFactory to support custom JSON serialization for exceptions.

[junalmeida]

This is also a thing for me because it invalidates the code in RetryOptions.Handle when using the method CallActivityWithRetryAsync.

Is there any estimates?

[ConnorMcMahon]

@junalmeida, we are unlikely to change our own serialization strategy, as that can have broad reaching implications. This is why we exposed the IErrorSerializerSettingsFactory via dependency injection, so that you can provide your own application with the serialization settings you need to handle errors thrown by your application in a robust way.

[junalmeida]

Currently my fast-track workaround is to catch the SqlException, determine if it is a transient one to then throw another exception that durable functions can handle.

I understand that a fix on that may cause side effects, however, I don't think keeping it as it is today is an acceptable path, because the internal serializer used causes an exception and obfuscates the real one, clearly a bug or at least an incompatibility between two Microsoft owned libraries. Maybe at least a plan to fix on the next major version release?

[ConnorMcMahon]

@junalmeida, I would challenge the notion that this is a bug, and assert this is a general limitation. Certain types do not serialize/deserialize cleanly, and one of the requirements for Durable Functions is that the data that is persisted (be that orchestration/activity inputs, outputs or exceptions) have to be serializable and deserializable. I think we need to more clearly document these limitations, but at the end of the day, there is not much we can do if the data given to us cannot be serialized/deserialized ina consistent way.

There are sets of settings with Newtonsoft that would potentially work with a wider variety of types that we could consider down the road in a Durable Functions v3.x (which is not planned anytime soon), but these often have a tradeoff of being larger strings and being more rigid and fragile to things like type renames. Essentially, serialization is a hard problem to make work in a generic fashion, so we chose what we deem to be reasonable defaults, and allow apps to customize the serialization/deserialization logic to their own needs.

We are looking into solutions that would handle this more gracefully, but for now, the answer is to provide your own serialization.

[ConnorMcMahon]

As for this specific exception, it looks like the owner of that library are working to make it work better with serialization in their new major version: https://github.com/dotnet/SqlClient/issues/524.

[junalmeida]

I understand that certain types can't serialize/deserialize cleanly, but we are not talking about user-generated code, we are talking about the very well known SqlException, and the impact that failing to deserialize it causes on the RetryOptions.Handle method, a piece of code supposed to be supported by this library, currently not fully working as expected due to this bug or limitation.

I agree with you that these limitations should be better documented, however do you think that stating that SqlException can't be thrown is something to be well received by your users? I don't think so, but this is a call on your team to decide. I can speak for myself and I don't agree that is acceptable to add a custom code to deal with a limitation between two libraries owned by same vendor, specially touching the serialization subject, one of the main foundations Durable Functions is built on top of.

Don't get me wrong, this library is perfect, works very well on a variety of scenarios I need today. I just gently disagree on a decision to not fixing a limitation caused by the combination of two libraries owned by the same vendor that are more than supposedly meant to be used together.

[ConnorMcMahon]

That's completely fair @junalmeida. My main hesitation is that there are alot of official Microsoft libraries, and making sure we serialize all of the exceptions from each of them (even just the most commonly used ones) would put a fairly large burden on us as a development team, especially since new libraries are being created all of the time.

Ideally, each library's developers should ensure that their data/exceptions is more easily serializable with common serialization libraries. In this specific case, it looks like the SqlException defined in versions greater than v2.0.0 of Microsoft.Data.SqlClient works much better with Newtonsoft than the version defined inthe legacy System.Data.SqlClient namespace.