RFC: Make JsonRpc to NOT swallow exceptions

[artempyanykh]

This is once again a continuation of #25.

There were several issues with the previous version.
Catching exceptions in server's code and turning them into LocalRpcExceptions probably isn't the best because:

1. Notifications don't require a response, hence no error will be shown
   to the user. Effectively, this means any exceptions including fatal
   can get swallowed.
2. The context of the original exception is lost in conversion.

This is a breaking change and may not be desirable for servers that use exception for control flow/recovery. But I think the spirit of the library is such that

• Recoverable (user reportable) errors are encoded as LspResult's Error.
• Exceptions are non-recoverable and should terminate the server.

Note: since now we're awaiting jsonRpc.Completion which completes when connection is closed, I could clean up a workaround for VSCode's incorrect shutdown sequence.

[artempyanykh]

As an example, with this change I'm getting an error and a stack trace like this in lsp logs, when the server hits failwith or other kind of unrecoverable error:

Unhandled exception: System.AggregateException: One or more errors occurred. (Exception has been thrown by the target of an invocation.)
 ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.Exception: Planted error!
   at Marksman.Server.MarksmanServer.TextDocumentDidChange(DidChangeTextDocumentParams par) in /Users/arr/dev/marksman/Marksman/Server.fs:line 411
   at Ionide.LanguageServerProtocol.Server.Pipe #1 input at line 183@187-4.Invoke(server s, DidChangeTextDocumentParams p) in /Users/arr/dev/marksman/LanguageServerProtocol/LanguageServerProtocol.fs:line 187

[baronfel]

Whereas before that same error would be transmitted to the client and just show the exception message?

[artempyanykh]

@baronfel not quite. In this case the exception happened in textDocument/didChange and previously it would just get swallowed by JsonRpc because notifications don't require a response.

They actually mention this here:

[baronfel]

Well overall I'd prefer to have the information, so let's take this change :)

[artempyanykh]

Coolio! 🤘 As I see it, the only questionable part in this PR is:

use jsonRpc =
       { new JsonRpc(jsonRpcHandler) with
           member this.IsFatalException(ex: Exception) = true }

In case dying on exceptions will prove to be no bueno, we could make this thing configurable.

[razzmatazz]

Coolio! 🤘 As I see it, the only questionable part in this PR is:

use jsonRpc =
       { new JsonRpc(jsonRpcHandler) with
           member this.IsFatalException(ex: Exception) = true }

In case dying on exceptions will prove to be no bueno, we could make this thing configurable.

uh.. there are actually some problems with this, regarding handling of LspResult.Error -- see #33