Propagate exceptions during message body deserialization

[ReubenBond]

In some cases, a message body cannot be deserialized and so the message is dropped without notifying the caller. The call will eventually fail with a timeout exception but with no indication of why that timeout occurred.

This PR propagates body deserialization exceptions back to the caller.

Instead of deserializing the body in the Message constructor, this PR moves the assignment of the body to a separate method. This way, the partially-deserialized message is still valid and has correctly deserialized headers, so a failure response can be constructed.

[gabikliot]

Are u sure? I remember very clearly this particular case of body can't deserialze and I fixed it to send exception. It was fixed and correct for years. If this is broken now, it's a regression introduced recently.

[ReubenBond]

@gabikliot there are two cases:

• One where deserialization is deferred until it's running in the context of an activation which is fine: we get msg.BodyObject at which point deserialization occurs and the exception is thrown and handled properly. This happens when messages are deserialized in the by the gateway receiver.
• The other case (which this fixes) in inter-silo communication where we eagerly deserialize the body so that we can immediately reclaim the underlying byte arrays.

This is a bug which I've known has existed for some time, but never pinned it down until now.

BTW, there's still a bug: when RequestContext contains an undeserializable object, this will still fail with a timeout because RequestContext is stored in the message headers.

EDIT: I might be wrong about my analyses there, but the bug is rather obvious in the TryDecodeMessage code.

[gabikliot]

I see.
It used to be that we only ever had the first case. Then the 2nd case was added, about a year - year and a half ago (@ jason-bragg). Very dangerous to have the main code path be split like this.

[ReubenBond]

@dotnet-bot test this please

[ReubenBond]

@gabikliot I'm assuming that this was done in the name of performance, but I certainly agree that the added complexity adds a bit of risk - it wasn't obvious to me why this was happening at first.

Is anyone available to review the fix?

[gabikliot]

I will try to take a look, but I really think this should be reviewed by @jason-bragg .

[jason-bragg]

Looks good. This code path is rather fragile, but it looks like all error cases have been properly handled.

[jason-bragg]

What is this used for? We try to avoid linq in this layer due to perf. Not saying to remove this, just don't see where it is used.

[ReubenBond]

Fixed, thanks :)