Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonRpcEventSource.GetArgumentsString may throw exceptions, causing failures while verbose logging #841

Closed
AArnott opened this issue Sep 8, 2022 · 2 comments · Fixed by #842
Assignees
Labels
Milestone

Comments

@AArnott
Copy link
Member

AArnott commented Sep 8, 2022

As reported in devdiv-1600609, we have errors with this message coming from StreamJsonRpc:

Deserializing JSON-RPC argument with name "" and position 1 to type "" failed: Failed to deserialize System.Object value.

The message itself comes from the JsonMessageFormatter:
https://github.com/AArnott/vs-streamjsonrpc/blob/39a07107ae2a2fcc0a4846a946b2489fb4197ad1/src/StreamJsonRpc/JsonMessageFormatter.cs#L1036-L1042
and the MessagePackFormatter:
https://github.com/AArnott/vs-streamjsonrpc/blob/39a07107ae2a2fcc0a4846a946b2489fb4197ad1/src/StreamJsonRpc/MessagePackFormatter.cs#L2460-L2466

This lack of detail (e.g. no argument name nor type hint) makes triaging bug reports very difficult. This lack of detail appears from code inspection to only come from JsonRpcEventSource.GetArgumentsString here:

if (request.TryGetArgumentByNameOrIndex(null, i, null, out object? value))

With a similarly useless but slightly different message coming from a few lines further down:

if (request.TryGetArgumentByNameOrIndex(key, -1, null, out object? value))

This GetArgumentsString is only called from a few places in the JsonRpc class, all within a check for Verbose logging being enabled. In fact, the GetArgumentsString method itself is only meant for logging the arguments. But evidently that attempt to represent the arguments as a string fails, and throws an exception, leading to the failure of the overall scenario.

We really shouldn't be failing the scenario for sake of failing to log. Let's make logging more resilient!

@AArnott AArnott added the bug label Sep 8, 2022
@AArnott AArnott self-assigned this Sep 9, 2022
@AArnott
Copy link
Member Author

AArnott commented Sep 9, 2022

The "Failed to deserialize System.Object value" part of the error doesn't appear in Newtonsoft.Json as far as I can tell. It does appear in messagepack however, so in attempting to repro this, I think we should try the MessagePackFormatter.

@AArnott
Copy link
Member Author

AArnott commented Sep 9, 2022

In fact, here is more info from the exception thrown:

StreamJsonRpc.RemoteInvocationException: Deserializing JSON-RPC argument with name "" and position 1 to type "" failed: Failed to deserialize System.Object value.
   at StreamJsonRpc.JsonRpc.d__143`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.d__16`1.MoveNext()
RPC server exception:
StreamJsonRpc.RpcArgumentDeserializationException: Deserializing JSON-RPC argument with name "" and position 1 to type "" failed: Failed to deserialize System.Object value.
 ---> MessagePack.MessagePackSerializationException: Failed to deserialize System.Object value.
 ---> MessagePack.MessagePackSerializationException: Invalid primitive bytes.
      at MessagePack.Formatters.PrimitiveObjectFormatter.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
      at MessagePack.Formatters.PrimitiveObjectFormatter.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
      at MessagePack.Formatters.PrimitiveObjectFormatter.DeserializeMap(MessagePackReader& reader, Int32 length, MessagePackSerializerOptions options)
      at MessagePack.Formatters.PrimitiveObjectFormatter.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
      at MessagePack.Formatters.PrimitiveObjectFormatter.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
      at MessagePack.MessagePackSerializer.Deserialize[T](MessagePackReader& reader, MessagePackSerializerOptions options)
   --- End of inner exception stack trace ---
      at MessagePack.MessagePackSerializer.Deserialize[T](MessagePackReader& reader, MessagePackSerializerOptions options)
      at StreamJsonRpc.MessagePackFormatter.JsonRpcRequest.TryGetArgumentByNameOrIndex(String name, Int32 position, Type typeHint, Object& value)
   --- End of inner exception stack trace ---
      at StreamJsonRpc.MessagePackFormatter.JsonRpcRequest.TryGetArgumentByNameOrIndex(String name, Int32 position, Type typeHint, Object& value)
      at StreamJsonRpc.JsonRpcEventSource.GetArgumentsString(JsonRpcRequest request)
      at StreamJsonRpc.JsonRpc.d__158.MoveNext()

AArnott added a commit to AArnott/vs-streamjsonrpc that referenced this issue Sep 9, 2022
AArnott added a commit to AArnott/vs-streamjsonrpc that referenced this issue Sep 9, 2022
@AArnott AArnott added this to the v2.13 milestone Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant