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

Allow server the option to crash on exception thrown #74

Merged
merged 22 commits into from Jan 12, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
de75371
update readme, allow server to crash on exception
jepetty Dec 14, 2017
43d9c91
remove temp exception in favor of taskCanceledException
jepetty Dec 15, 2017
d5f7435
revert change in statement order
jepetty Dec 15, 2017
226db0a
resolve merge conflicts
jepetty Dec 15, 2017
0bbc033
separate overloaded rpc tests and remove unnecessary tests
jepetty Dec 18, 2017
673d727
update API to include exception as parameter
jepetty Dec 19, 2017
ef74c1a
add parameter documentation
jepetty Dec 19, 2017
8b9678c
update documentation for new functionality
jepetty Dec 20, 2017
8152cf4
improve tests and add remarks
jepetty Dec 20, 2017
3b1efda
Only pass inner exception through filter, not TargetInvocationExcepti…
jepetty Dec 20, 2017
5994ffb
update docs as per cr feedback
jepetty Dec 20, 2017
747af41
update all parts of doc to instantiate JsonRpc twice
jepetty Dec 20, 2017
5435f72
closing streams on operation canceled, other pr comments
jepetty Dec 20, 2017
97f2c56
Move IsDisposed to FullDuplexStreams and check streams disposed in Me…
jepetty Dec 20, 2017
89a602e
fix warnings in error list
jepetty Dec 20, 2017
dbde851
fix build error
jepetty Jan 4, 2018
3d04487
update documentation, only filter exceptions once
jepetty Jan 8, 2018
12c6c54
Remove obsolete comment since we don't send cancellation exceptions t…
AArnott Jan 10, 2018
22a1a68
Add a few tests around AggregateException
AArnott Jan 10, 2018
a0477dd
responding to pr feedback
jepetty Jan 11, 2018
ae643fa
merge master branch
jepetty Jan 11, 2018
984c20c
fix resx file tag, make test always async
jepetty Jan 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 33 additions & 58 deletions doc/index.md
Expand Up @@ -5,78 +5,65 @@ There are two ways to establish connection and start invoking methods remotely:

1. Use the static `Attach` method in `JsonRpc` class:
```csharp
public void ConstructRpc()
public void ConstructRpc(Stream stream)
{
var target = new LanguageServerTarget();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
var rpc = JsonRpc.Attach(stream, target);
}
```
The `JsonRpc` object returned by `Attach` method would be used to invoke remote methods via JSON-RPC.

2. Construct a `JsonRpc` object directly:
```csharp
public void ConstructRpc()
public void ConstructRpc(Stream clientStream, Stream serverStream)
{
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = new JsonRpc(streams.Item1);
var serverRpc = new JsonRpc(streams.Item2);
var clientRpc = new JsonRpc(clientStream, serverStream);
var target = new Server();
serverRpc.AddLocalRpcTarget(target);
clientRpc.StartListening();
serverRpc.StartListening();
rpc.AddLocalRpcTarget(target);
rpc.StartListening();
}
```

## Invoking a notification
To invoke a remote method named "foo" which takes one `string` parameter and does not return anything (i.e. send a notification remotely):
```csharp
public async Task NotifyRemote()
public async Task NotifyRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
await clientRpc.NotifyAsync("foo", "param1");
var rpc = JsonRpc.Attach(stream, target);
await rpc.NotifyAsync("foo", "param1");
}
```
The parameter will be passed remotely as an array of one object.

To invoke a remote method named "bar" which takes one `string` parameter (but the parameter should be passed as an object instead of an array of one object):
```csharp
public async Task NotifyRemote()
public async Task NotifyRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
await clientRpc.NotifyWithParameterObjectAsync("bar", "param1");
var rpc = JsonRpc.Attach(stream, target);
await rpc.NotifyWithParameterObjectAsync("bar", "param1");
}
```
## Invoking a request
To invoke a remote method named "foo" which takes two `string` parameters and returns an int:
```csharp
public async Task NotifyRemote()
public async Task InvokeRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
var myResult = await clientRpc.InvokeAsync<int>("foo", "param1", "param2");
var rpc = JsonRpc.Attach(stream, target);
var myResult = await rpc.InvokeAsync<int>("foo", "param1", "param2");
}
```
The parameters will be passed remotely as an array of objects.

To invoke a remote method named "baz" which takes one `string` parameter (but the parameter should be passed as an object instead of an array of one object) and returns a string:
```csharp
public async Task NotifyRemote()
public async Task InvokeRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
var myResult = await clientRpc.InvokeWithParameterObjectAsync<string>("baz", "param1");
var rpc = JsonRpc.Attach(stream, target);
var myResult = await rpc.InvokeWithParameterObjectAsync<string>("baz", "param1");
}
```

Expand All @@ -98,13 +85,11 @@ public class Server

public class Connection
{
public async Task NotifyRemote()
public async Task InvokeRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
var myResult = await clientRpc.InvokeWithParameterObjectAsync<string>("baz", "param1");
var rpc = JsonRpc.Attach(stream, target);
var myResult = await rpc.InvokeWithParameterObjectAsync<string>("baz", "param1");
}
}
```
Expand All @@ -120,53 +105,43 @@ public class Server : BaseClass

public class Connection
{
public async Task NotifyRemote()
public async Task InvokeRemote(Stream stream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStream.CreateStreams();
var clientRpc = JsonRpc.Attach(streams.Item1);
var serverRpc = JsonRpc.Attach(streams.Item2, target);
var myResult = await clientRpc.InvokeWithParameterObjectAsync<string>("test/InvokeTestMethod");
var rpc = JsonRpc.Attach(stream, target);
var myResult = await rpc.InvokeWithParameterObjectAsync<string>("test/InvokeTestMethod");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having just one JsonRpc instance created looks much better (more real). Thanks.

However, one can't use the same JsonRpc instance that you passed the server to to also invoke methods on that server. You'd need the JsonRpc instance that the client created to invoke methods on the server.
Can the docs show how it looks on the server, and then as a separate code snippet show "and with this attribute on the server method, the client can now invoke this method with this special name, like this..."

}
}
```

## Crashing the process on exception
In some cases, you may want to immediately crash the server process if certain exceptions are thrown. In this case, overriding the `IsFatalException` method will give you the desired functionality. Through `IsFatalException` you can access and respond to exceptions as they are observed.
## Close stream on fatal errors
In some cases, you may want to immediately close the streams if certain exceptions are thrown. In this case, overriding the `IsFatalException` method will give you the desired functionality. Through `IsFatalException` you can access and respond to exceptions as they are observed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the subject of closing the stream, we should think about how this fits with the new WebSocket transport that we now support, where closing the connection involves sending an async message before actually disposing the connection. But as closing the stream on failure is not new functionality, reconciling these doesn't have to be in this PR, IMO.

```csharp
public class Server : BaseClass
{
public void ThrowsException() => throw new Exception("Throwing an exception");
}

public class JsonRpcCrashesOnException : JsonRpc
public class JsonRpcClosesStreamOnException : JsonRpc
{
public JsonRpcCrashesOnException(Stream clientStream, Stream serverStream, object target = null) : base(clientSteam, serverStream, target)
public JsonRpcClosesStreamOnException(Stream clientStream, Stream serverStream, object target = null) : base(clientStream, serverStream, target)
{
}

protected override bool IsFatalException(Exception ex)
{
if !(ex is OperationCanceledException)
{
Environment.FailFast(ex.message, ex);
}

return false;
return true;
}
}

public class Connection
{
public async Task NotifyRemote()
public async Task InvokeRemote(Stream clientStream, Stream serverStream)
{
var target = new Server();
var streams = Nerdbank.FullDuplexStreams.CreateStreams();
var clientRpc = new JsonRpcCrashesOnException(streams.Item1);
var serverRpc = new JsonRpcCrashesOnException(streams.Item2, target);
clientRpc.StartListening();
serverRpc.StartListening();
await clientRpc.InvokeAsync(nameof(Server.ThrowsException));
var rpc = new JsonRpcClosesStreamOnException(clientStream, serverStream, target);
rpc.StartListening();
await rpc.InvokeAsync(nameof(Server.ThrowsException));
}
}
```
57 changes: 17 additions & 40 deletions src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs
Expand Up @@ -38,18 +38,17 @@ public async Task CanInvokeMethodOnServer()
}

[Fact]
public async Task CloseStreamsIfCancelledTaskReturned()
public async Task StreamsStayOpenIfCancelledTaskReturned()
{
var ex = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.ServerMethodThatReturnsCancelledTask)));
Assert.Equal(CancellationToken.None, ex.CancellationToken);
Assert.NotNull(ex.StackTrace);
Assert.Equal("The operation was canceled.", this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(0, this.serverRpc.IsFatalExceptionCount);

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Assert.True(((IDisposableObservable)this.serverRpc).IsDisposed);
Assert.True(((DisposingMessageHandler)this.messageHandler).IsDisposed);
// Assert that the JsonRpc and MessageHandler objects are not disposed
Assert.False(((IDisposableObservable)this.clientRpc).IsDisposed);
Assert.False(((IDisposableObservable)this.serverRpc).IsDisposed);
Assert.False(((DisposingMessageHandler)this.messageHandler).IsDisposed);
}

[Fact]
Expand All @@ -74,7 +73,7 @@ public async Task CloseStreamOnAsyncYieldAndThrowException()
Exception exception = await Assert.ThrowsAnyAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.AsyncMethodThatThrowsAfterYield), exceptionMessage));
Assert.NotNull(exception.StackTrace);
Assert.Equal(exceptionMessage, this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(1, this.serverRpc.IsFatalExceptionCount);

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Expand All @@ -89,21 +88,7 @@ public async Task CloseStreamOnAsyncThrowExceptionandYield()
Exception exception = await Assert.ThrowsAnyAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.AsyncMethodThatThrowsBeforeYield), exceptionMessage));
Assert.NotNull(exception.StackTrace);
Assert.Equal(exceptionMessage, this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Assert.True(((IDisposableObservable)this.serverRpc).IsDisposed);
Assert.True(((DisposingMessageHandler)this.clientRpc.MessageHandler).IsDisposed);
}

[Fact]
public async Task CloseStreamOnAsyncMethodException()
{
var exceptionMessage = "Exception from CloseStreamOnAsyncMethodException";
await Assert.ThrowsAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.MethodThatThrowsAsync), exceptionMessage));
Assert.Equal(exceptionMessage, this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(1, this.serverRpc.IsFatalExceptionCount);

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Expand All @@ -117,7 +102,7 @@ public async Task CloseStreamOnAsyncTMethodException()
var exceptionMessage = "Exception from CloseStreamOnAsyncTMethodException";
await Assert.ThrowsAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.AsyncMethodThatReturnsStringAndThrows), exceptionMessage));
Assert.Equal(exceptionMessage, this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(1, this.serverRpc.IsFatalExceptionCount);

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Expand All @@ -135,7 +120,7 @@ public async Task StreamsStayOpenForNonServerException()
}

[Fact]
public async Task CloseStreamsOnOperationCanceled()
public async Task StreamsStayOpenOnOperationCanceled()
{
using (var cts = new CancellationTokenSource())
{
Expand All @@ -145,13 +130,13 @@ public async Task CloseStreamsOnOperationCanceled()

// Ultimately, the server throws because it was canceled.
var ex = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => invokeTask.WithTimeout(UnexpectedTimeout));
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(0, this.serverRpc.IsFatalExceptionCount);
}

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Assert.True(((IDisposableObservable)this.clientRpc).IsDisposed);
Assert.True(((IDisposableObservable)this.serverRpc).IsDisposed);
Assert.True(((DisposingMessageHandler)this.clientRpc.MessageHandler).IsDisposed);
// Assert that the JsonRpc and MessageHandler objects are not disposed
Assert.False(((IDisposableObservable)this.clientRpc).IsDisposed);
Assert.False(((IDisposableObservable)this.serverRpc).IsDisposed);
Assert.False(((DisposingMessageHandler)this.clientRpc.MessageHandler).IsDisposed);
}

[Fact]
Expand All @@ -166,7 +151,7 @@ public async Task CancelMayStillReturnErrorFromServer()

await Assert.ThrowsAsync<TaskCanceledException>(() => invokeTask);
Assert.Equal(Server.ThrowAfterCancellationMessage, this.serverRpc.FaultException.Message);
Assert.Equal(2, this.serverRpc.IsFatalExceptionCount);
Assert.Equal(1, this.serverRpc.IsFatalExceptionCount);
}

// Assert that the JsonRpc and MessageHandler objects are disposed after exception
Expand Down Expand Up @@ -219,7 +204,7 @@ public async Task<string> AsyncMethodThatReturnsStringAndThrows(string message)
{
await Task.Run(() => throw new Exception(message));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite to this to guarantee the method is always asynchronous.

await Task.Yield();
throw new Exception(message);


return "never will return";
return await Task.FromResult("never will return");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you're using return await Task.FromResult(x) instead of just return x. I can't think of any difference that makes at runtime and it seems more verbose.

}

public async Task<string> AsyncMethodWithCancellation(string arg, CancellationToken cancellationToken)
Expand Down Expand Up @@ -251,11 +236,6 @@ public async Task<string> AsyncMethodFaultsAfterCancellation(string arg, Cancell

throw new InvalidOperationException(ThrowAfterCancellationMessage);
}

public async Task MethodThatThrowsAsync(string message)
{
await Task.Run(() => throw new Exception(message));
}
}

public class DisposingMessageHandler : HeaderDelimitedMessageHandler
Expand All @@ -282,9 +262,6 @@ internal class JsonRpcWithFatalExceptions : JsonRpc
{
internal Exception FaultException;

// If the exception arises from a task faulting or canceling, this method will be called twice:
// once in the handling of the task and once when dispatching the request
// If the exception arises from a synchronous method call, this method will be called once when dispatching the request
internal int IsFatalExceptionCount;

public JsonRpcWithFatalExceptions(DelimitedMessageHandler messageHandler, object target = null)
Expand Down
5 changes: 5 additions & 0 deletions src/StreamJsonRpc/EventArgs/DisconnectedReason.cs
Expand Up @@ -27,5 +27,10 @@ public enum DisconnectedReason
/// The stream was disposed.
/// </summary>
Disposed,

/// <summary>
/// A fatal exception was thrown from the server method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since for most readers they probably have a distinct idea of which end the "server" refers to, and yet this actually always refers to the local side, perhaps we should rephrase this to:

A fatal exception was thrown in a local method that was requested by the remote party.

/// </summary>
FatalException,
}
}