Issues with MessagePack implementation of IHubProtocol

[AArnott]

Describe the bug

In writing a Nerdbank.MessagePack implementation of IHubProtocol, I've been studying SignalR's existing implementation based on MessagePack-CSharp. I've noticed several potential issues that I will collect here for your consideration:

Real bugs

1. Serialization requires knowing the declared types of arguments and return values. It is not enough to simply call object.GetType() and serialize with that. We need to know that the parameter is typed as BaseType and that the argument is an instance of DerivedType so that [Union] attributes on BaseType can be serialized. As you have it here where the serializer relies on GetType on the value to be serialized, a parameter or return type declared as BaseType but has an instance of DerivedType will serialize as a 'naked' DerivedType. But upon deserialization where we do have BaseType as a known type, the deserializer will look for a type disambiguating header to precede the DerivedType data. But it won't be there because BaseType wasn't a known type at serialization time. As a result, your SignalR implementation will fail when users try to send types with [Union] attributes on them. It also means my own implementation for Nerdbank.MessagePack will have the same bug, because your IHubProtocol interface simply doesn't make this information available.
2. The spec mandates a fixed protocol version of 1. But your code currently sets it to be 2. And while your code allegedly accepts a version 1 as compatible, if the version is exchanged both directions, surely your version 1 implementation on the other side will reject version 2 anyway. Am I missing something?

Other issues for your consideration

1. The protocol version being part of the protocol really ought to be communicated with the IHubProtocol implementation. At the moment, all the protocol version evidently does is to serve as an accept/reject decision. But it would be much more useful if the IHubProtocol also knew which version the remote party was using so that any necessary adjustments could be made in the protocol.
2. The message deserializing functions do not adequately guard against reading too much. While it calls ReadArrayHeader, most paths disregard its value and read as many elements as they want. In fact the only paths that use the return value are those that expect an optional value in the array. This issue is a lower priority than it would otherwise be because the MessagePackReader works off of a bounded buffer so that it cannot read more than one message. So this is ok for now.
3. The message deserializing functions do not guard against reading too little. Typically when reading an array, you should make sure you've read or skipped every single element in the array. Again, the fact that the top-level read function has guaranteed to have consumed the entire envelope makes this less of a manifesting bug. But it does prevent you from noticing when there's garbage at the end of a message.
4. The schema is fundamentally incompatible with using standard serialize/deserialize functions from msgpack libraries because it mixes both the disambiguating 'type' integer into the same array that stores the object's fields. This makes it impossible to have each message serialize/deserialize itself, and have a higher-level method write/read the message type identifier first. The best practice here would be something like this: [1, [message, fields, here]] instead of [1, message, fields, here]. Fixing this would be a protocol breaking change.

Expected Behavior

Ideally a messagepack protocol that follows best practices in the messagepack community.

[MihuBot]

Possible related and/or duplicate issues:

• [SignalR] Support union in the js client #7298
• SignalR MessagePack polymorphism. #30096

[AArnott]

One of the issues above is indeed a duplicate of #30096, which sadly appears to have been closed without understanding how SignalR needs to change to make it work. Hopefully my description above will help.