Fixed serialization of exceptions without the default constructor (#64). #76
Conversation
| var className = stream.ReadString(session); | ||
| var message = stream.ReadString(session); | ||
| var remoteStackTraceString = stream.ReadString(session); | ||
| var stackTraceString = stream.ReadString(session); | ||
| var innerException = stream.ReadObject(session); | ||
|
|
||
| var exception = hasDefaultConstructor ? Activator.CreateInstance(type) : | ||
| FormatterServicesGetUninitializedObject(type); |
There was a problem hiding this comment.
You don't need this check here - it's not like having default constructor or not will change depending on the binary payload ;) You can move this check outside the deserializer function i.e:
Func<Object> initialize = hasDefaultConstructor
? (() => Activator.CreateInstance(type))
: (FormatterServicesGetUninitializedObject(type));or (without measures I'm not sure which one is faster):
Func<Type, Object> initialize = hasDefaultConstructor
? Activator.CreateInstance
: FormatterServicesGetUninitializedObject;and use initialize inside deserializer.
There was a problem hiding this comment.
But now it introduces unnecessary delegate allocation.
There was a problem hiding this comment.
It happens once per exception type, while constructor check will happen once per each object deserialization call. In practice to be 100% sure, we'd need to check both approaches in performance test - I'll be happy to do that once #57 will finally get merged.
There was a problem hiding this comment.
You're right, thanks for the suggestion.
|
@Horusiath does this look ok to you now? |
Using
FormatterServices.GetUninitializedObjectinstead ofActivator.CreateInstancefor exception types that don't have a parameterless constructor.Reflection-based invocation of
GetUninitializedObjectcan be removed once the API is exposed in dotnet core API. See this thread for more info: https://github.com/dotnet/corefx/issues/8133