Skip to content

Commit 5f3a5ac

Browse files
david-driscolldavid-driscoll
authored
Fixed case where if serialization fails the server will die silently. (#447)
1 parent 21ebc56 commit 5f3a5ac

File tree

3 files changed

+138
-31
lines changed

3 files changed

+138
-31
lines changed

‎src/JsonRpc/InputHandler.cs

+55-30
Original file line numberDiff line numberDiff line change
@@ -411,53 +411,78 @@ private void HandleRequest(in ReadOnlySequence<byte> request)
411411
{
412412
if (item.IsRequest && item.Request != null)
413413
{
414-
// _logger.LogDebug("Handling Request {Method} {ResponseId}", item.Request.Method, item.Request.Id);
415-
var descriptor = _requestRouter.GetDescriptors(item.Request);
416-
if (descriptor.Default is null)
414+
try
417415
{
418-
_logger.LogDebug("Request handler was not found (or not setup) {Method} {ResponseId}", item.Request.Method, item.Request.Id);
419-
_outputHandler.Send(new MethodNotFound(item.Request.Id, item.Request.Method));
420-
return;
421-
}
416+
// _logger.LogDebug("Handling Request {Method} {ResponseId}", item.Request.Method, item.Request.Id);
417+
var descriptor = _requestRouter.GetDescriptors(item.Request);
418+
if (descriptor.Default is null)
419+
{
420+
_logger.LogDebug("Request handler was not found (or not setup) {Method} {ResponseId}", item.Request.Method, item.Request.Id);
421+
_outputHandler.Send(new MethodNotFound(item.Request.Id, item.Request.Method));
422+
return;
423+
}
422424

423-
var type = _requestProcessIdentifier.Identify(descriptor.Default);
424-
_scheduler.Add(type, $"{item.Request.Method}:{item.Request.Id}", RouteRequest(descriptor, item.Request));
425+
var type = _requestProcessIdentifier.Identify(descriptor.Default);
426+
_scheduler.Add(type, $"{item.Request.Method}:{item.Request.Id}", RouteRequest(descriptor, item.Request));
427+
}
428+
catch (JsonReaderException e)
429+
{
430+
_outputHandler.Send(new ParseError(item.Request.Id, item.Request.Method));
431+
_logger.LogCritical(e, "Error parsing request");
432+
}
433+
catch (Exception e)
434+
{
435+
_outputHandler.Send(new InternalError(item.Request.Id, item.Request.Method));
436+
_logger.LogCritical(e, "Unknown error handling request");
437+
}
425438
}
426439

427440
if (item.IsNotification && item.Notification != null)
428441
{
429-
// We need to special case cancellation so that we can cancel any request that is currently in flight.
430-
if (item.Notification.Method == JsonRpcNames.CancelRequest)
442+
try
431443
{
432-
_logger.LogDebug("Found cancellation request {Method}", item.Notification.Method);
433-
var cancelParams = item.Notification.Params?.ToObject<CancelParams>();
434-
if (cancelParams == null)
444+
// We need to special case cancellation so that we can cancel any request that is currently in flight.
445+
if (item.Notification.Method == JsonRpcNames.CancelRequest)
435446
{
436-
_logger.LogDebug("Got incorrect cancellation params", item.Notification.Method);
447+
_logger.LogDebug("Found cancellation request {Method}", item.Notification.Method);
448+
var cancelParams = item.Notification.Params?.ToObject<CancelParams>();
449+
if (cancelParams == null)
450+
{
451+
_logger.LogDebug("Got incorrect cancellation params", item.Notification.Method);
452+
continue;
453+
}
454+
455+
_logger.LogDebug("Cancelling pending request", item.Notification.Method);
456+
if (_requests.TryGetValue(cancelParams.Id, out var d))
457+
{
458+
d.cancellationTokenSource.Cancel();
459+
}
460+
437461
continue;
438462
}
439463

440-
_logger.LogDebug("Cancelling pending request", item.Notification.Method);
441-
if (_requests.TryGetValue(cancelParams.Id, out var d))
464+
// _logger.LogDebug("Handling Request {Method}", item.Notification.Method);
465+
var descriptor = _requestRouter.GetDescriptors(item.Notification);
466+
if (descriptor.Default is null)
442467
{
443-
d.cancellationTokenSource.Cancel();
468+
_logger.LogDebug("Notification handler was not found (or not setup) {Method}", item.Notification.Method);
469+
// TODO: Figure out a good way to send this feedback back.
470+
// _outputHandler.Send(new RpcError(null, new ErrorMessage(-32601, $"Method not found - {item.Notification.Method}")));
471+
return;
444472
}
445473

446-
continue;
447-
}
474+
var type = _requestProcessIdentifier.Identify(descriptor.Default);
475+
_scheduler.Add(type, item.Notification.Method, RouteNotification(descriptor, item.Notification));
448476

449-
// _logger.LogDebug("Handling Request {Method}", item.Notification.Method);
450-
var descriptor = _requestRouter.GetDescriptors(item.Notification);
451-
if (descriptor.Default is null)
477+
}
478+
catch (JsonReaderException e)
452479
{
453-
_logger.LogDebug("Notification handler was not found (or not setup) {Method}", item.Notification.Method);
454-
// TODO: Figure out a good way to send this feedback back.
455-
// _outputHandler.Send(new RpcError(null, new ErrorMessage(-32601, $"Method not found - {item.Notification.Method}")));
456-
return;
480+
_logger.LogCritical(e, "Error parsing notification");
481+
}
482+
catch (Exception e)
483+
{
484+
_logger.LogCritical(e, "Unknown error handling notification");
457485
}
458-
459-
var type = _requestProcessIdentifier.Identify(descriptor.Default);
460-
_scheduler.Add(type, item.Notification.Method, RouteNotification(descriptor, item.Notification));
461486
}
462487

463488
if (item.IsError)

‎test/Lsp.Tests/Integration/ErroringHandlingTests.cs

+82
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
using System;
2+
using System.Threading;
3+
using System.Threading.Tasks;
4+
using FluentAssertions;
5+
using NSubstitute;
6+
using OmniSharp.Extensions.JsonRpc.Server;
7+
using OmniSharp.Extensions.JsonRpc.Testing;
8+
using OmniSharp.Extensions.LanguageProtocol.Testing;
9+
using OmniSharp.Extensions.LanguageServer.Client;
10+
using OmniSharp.Extensions.LanguageServer.Protocol;
11+
using OmniSharp.Extensions.LanguageServer.Server;
12+
using OmniSharp.Extensions.LanguageServer.Protocol.Document;
13+
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
14+
using Xunit;
15+
using Xunit.Abstractions;
16+
17+
namespace Lsp.Tests.Integration
18+
{
19+
public class ErroringHandlingTests : LanguageProtocolTestBase
20+
{
21+
public ErroringHandlingTests(ITestOutputHelper outputHelper) : base(new JsonRpcTestOptions().ConfigureForXUnit(outputHelper))
22+
{
23+
}
24+
25+
[Fact]
26+
public async Task Should_Handle_Malformed_Request()
27+
{
28+
var (client, server) = await Initialize(ConfigureClient, ConfigureServer);
29+
30+
var codeActionParams = new {
31+
Range = new {
32+
Start = new { Line = 1, Character = double.Parse("1.0E300") },
33+
End = new { Line = 2, Character = 9999999999999999999 }
34+
},
35+
TextDocument = new {
36+
Uri = DocumentUri.From("/path/to/file")
37+
},
38+
Context = new {
39+
Diagnostics = new Container<Diagnostic>()
40+
}
41+
};
42+
43+
var req = client.SendRequest(TextDocumentNames.CodeAction, codeActionParams);
44+
Func<Task> a = () => req.Returning<object>(CancellationToken);
45+
a.Should().Throw<ParseErrorException>();
46+
}
47+
48+
[Fact]
49+
public async Task Should_Handle_Malformed_Notification()
50+
{
51+
var (client, server) = await Initialize(ConfigureClient, ConfigureServer);
52+
53+
var notification = new {
54+
ContentChanges = new[] {
55+
new {
56+
Text = "Text change",
57+
Range = new {
58+
Start = new { Line = 1, Character = double.Parse("1.0E300") },
59+
End = new { Line = 2, Character = 9999999999999999999 }
60+
},
61+
}
62+
},
63+
TextDocument = new {
64+
Uri = DocumentUri.From("/path/to/file")
65+
},
66+
};
67+
68+
Action a = () => client.SendNotification(TextDocumentNames.DidChange, notification);
69+
a.Should().NotThrow();
70+
}
71+
72+
73+
private void ConfigureClient(LanguageClientOptions options)
74+
{
75+
}
76+
77+
private void ConfigureServer(LanguageServerOptions options)
78+
{
79+
options.OnCodeAction(@params => Task.FromResult(new CommandOrCodeActionContainer()), (capability, capabilities) => new CodeActionRegistrationOptions());
80+
}
81+
}
82+
}

‎test/Lsp.Tests/Integration/LanguageServerConfigurationTests.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ public async Task Should_Support_Options()
233233
options.Value.Port.Should().Be(443);
234234
}
235235

236-
[Fact]
236+
[RetryFact]
237237
public async Task Should_Support_Options_Monitor()
238238
{
239239
var (_, server, configuration) = await InitializeWithConfiguration(ConfigureClient, options => {

0 commit comments

Comments
 (0)