NullReferenceException in Receiver.GetRenor if "params" property is present but null

[tintoy]

Hi.

Found a small problem can can silently crash the language server's receiver so it simply stops processing messages. Basically, if you send the server a message like:

{
    "jsonrpc": "2.0",
    "id": "1234",
    "method": "foo",
    "params": null
}

Then the following line crashes with a NullReferenceException because hasParams is true, but @params is null:

csharp-language-server-protocol/src/JsonRpc/Reciever.cs

Line 81 in b1f0d7e

if (hasParams && @params.Type != JTokenType.Array && @params.Type != JTokenType.Object)

More generally, it looks like the receiver should probably have a try/catch around each receive cycle.

I'll open 2 PRs to separately deal with the NullReferenceException and the more-general try/catch.

[david-driscoll]

I think with my proposed change we can avoid a try catch over everything, but it may be useful to avoid unhandled exceptions, I need to think about it. It may also mask the true issue if there is a problem when reading a message from the client. In this scenario it was a null params which is technically invalid as per the spec, but we're just missing the case where it can "have" params but they are null instead of a JToken.

[tintoy]

Yeah, I think it's all good as long as a bad packet doesn't silently "crash" the server :)

[tintoy]

Thanks for the feedback on the PR by the way, will get onto that first thing this morning

[tintoy]

BTW, as it stands, this stuff is a little tricky to troubleshoot (especially with cross-platform clients, e.g. vscode-languageclient, LSP4j, etc) because there's no logging; I did create a fork of the library with logging added just for diagnostic purposes (helped me track down quite a few expectation-mismatches), but it might be worth considering adding some sort of logging at some point, too.

FWIW, the LSP client library I built has extensive logging especially around errors encountered during send / receive / dispatch (feel free to reuse anything that looks like it might be helpful).

[david-driscoll]

Logging was something I considered but it got confusing because if we log to console, then the client gets confused trying to parse things out. Writing too a file however would work better. I'll make an issue.

[tintoy]

BTW, I used Serilog, but there's always Microsoft.Extensions.Logging or LibLog if you want to stay framework-agnostic.

There's always logging to STDERR, as well if all else fails :)

[david-driscoll]

Closed via #25