Improve/Fix GetJsonText on built-in formatters #386
Comments
AArnott
added a commit
to AArnott/vs-streamjsonrpc
that referenced
this issue
Dec 5, 2019
We have JsonConverter and IMessagePackFormatter<T> types that have side effects in order to support marshaling of special objects. This makes tracing the full JSON-RPC message hazardous if it means we run these custom serializers multiple times per message, and this is in fact happening lately. The fix is to be sure we only serialize/deserialize a message *once*. This means that the result of the serialization must be retained long enough to trace (if tracing is turned on). It also means we need to retain the text that was *de*serialized long enough to be traced. We also don't want to allocate a string for the entire message unless tracing requires it. The approach varies between formatters, as follows: For the `JsonMessageFormatter` we have a super-convenient `JToken` instance that represents the entire message either after serialization or before deserialization. This type knows how to render a string representation of itself. So for this formatter we simply raise the trace events with the `JToken` as a parameter, which the string formatter will call `ToString` on if a trace listener wants it as a string. For the `MessagePackFormatter` it's a little more complicated. There is no native JSON text representation at all (since msgpack is a binary encoding). But we still want to trace a JSON text representation for logging/diagnostics purposes. MessagePack has a converter that translates msgpack binary to JSON text, but this requires that we have the entire msgpack byte sequence. When deserializing this is easy enough: we have the sequence to deserialize from anyway. But when serializing, we're writing to an `IBufferWriter<T>` which doesn't give us access to access the serialized binary later. So seeing the full msgpack that was serialized in order to convert to JSON and trace it is something the formatter needs help with. The message handlers have access to the full msgpack to be written, so we arrange for the handlers to "call back" into the formatter after the formatter is done serializing in order to say "by the way, here's the full sequence you just wrote out" which the formatter can then use to raise the trace event with an object that will convert it to JSON text if/when the object's ToString() method is called. This call back from the handler into the formatter is through an optional interface that only the `MessagePackFormatter` needs to implement. As part of this 'callback' from handler to formatter, the `LengthHeaderMessageHandler` needed a slightly revision: it didn't have a way to collect the entire serialized sequence written by the formatter because the `PrefixingBufferWriter<T>` doesn't expose a `ReadOnlySequence<T>` for all the written bytes like `Sequence<T>` does. So I had to switch to `Sequence<T>` in this handler. This means that small messages will have their buffer copied (even when not tracing) once before being transmitted where they weren't being copied before. But these are small messages so the impact is likely very small. Large messages were already getting copied anyway, so no difference there. So with this change we now have safe and complete tracing of JSON-RPC messages for all handlers and formatters, and without nasty doubling of side-effects. Fixes microsoft#386
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
JsonMessageFormatter.GetJsonTextmethod is woefully incomplete. It doesn't use any custom serializers at all and as a result Verbose tracing is misleading since the messages are missing data without warning. Switching this to use the regularSerializemethod leads to side-effects that actually break behavior (e.g. cancellation breaks).The
MessagePackFormatter.GetJsonTextmethod has all the side-effects of the custom formatters that are applied to it (e.g. streams, enumerables, etc). So while it is accurate, it leads to failures when those side-effects may be repeated between tracing and actual transmission. There's also no guarantee that such side effects won't leak resources if the message isn't actually transmitted.We can either:
The text was updated successfully, but these errors were encountered: