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 7 commits
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
2 changes: 1 addition & 1 deletion README.md
Expand Up @@ -34,6 +34,6 @@ Testing this library or users of this library can be done without any transport
by using the [Nerdbank.FullDuplexStream][FullDuplexStream] library in your tests
to produce the Stream object.

[JSONRPC]: http://json-rpc.org/
[JSONRPC]: http://jsonrpc.org/
Copy link
Member

Choose a reason for hiding this comment

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

This is a great change. We should get the docs fixed in the oldest servicing branch we have. Can you author a commit based on the v1.2 branch and send out a PR for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. I was also wondering if it's worth adding more explicit documented examples of this new functionality, or if the tests are sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

I think a new topic under the doc folder is a great idea.

[json-rpc-peer]: https://www.npmjs.com/package/json-rpc-peer
[FullDuplexStream]: https://www.nuget.org/packages/nerdbank.fullduplexstream
253 changes: 253 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs
@@ -0,0 +1,253 @@
using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.Threading;
using Nerdbank;
using Newtonsoft.Json;
using StreamJsonRpc;
using Xunit;
using Xunit.Abstractions;
using static JsonRpcMethodAttributeTests;

public class JsonRpcOverloadTests : TestBase
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest you add only tests that focus on the behavior that you intend to change, or that is at particular risk of regressing with your change that don't have adequate coverage already.
I see some very specialized tests below (e.g. that test race conditions for cancellation) that don't seem at all relevant to your exception filter.

Copy link
Member

Choose a reason for hiding this comment

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

Let's be sure to cover sync methods, async Task methods as well as async Task<T> methods, including cases that throw before and after their first yielding await. If the nested Server class you define here focuses on those scenarios, and test methods are named after those scenarios, I think it will be much clearer that we're covering the interesting scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't see any tests that assert that the streams are actually closed after the fatal exception. That's something of the core of your change, so I believe each test should assert that as well.

{
private readonly Server server;
private readonly FullDuplexStream serverStream;
private readonly FullDuplexStream clientStream;
private readonly JsonRpc clientRpc;

public JsonRpcOverloadTests(ITestOutputHelper logger)
: base(logger)
{
this.server = new Server();
var streams = FullDuplexStream.CreateStreams();
this.serverStream = streams.Item1;
this.clientStream = streams.Item2;

this.clientRpc = new JsonRpcOverload(this.clientStream, this.serverStream, this.server);
this.clientRpc.StartListening();
}

[Fact]
public async Task CanInvokeMethodOnServer()
{
string testLine = "TestLine1" + new string('a', 1024 * 1024);
string result1 = await this.clientRpc.InvokeAsync<string>(nameof(Server.ServerMethod), testLine);
Assert.Equal(testLine + "!", result1);
}

[Fact]
public async Task CanInvokeMethodThatReturnsCancelledTask()
{
var ex = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.ServerMethodThatReturnsCancelledTask)));
Assert.Equal(CancellationToken.None, ex.CancellationToken);
}

[Fact]
public async Task CannotPassExceptionFromServer()
{
OperationCanceledException exception = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.MethodThatThrowsUnauthorizedAccessException)));
Assert.NotNull(exception.StackTrace);

await Assert.ThrowsAsync<ObjectDisposedException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.ServerMethod), "testing"));
}

[Fact]
public async Task CanCallAsyncMethodThatThrows()
{
Exception exception = await Assert.ThrowsAnyAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.AsyncMethodThatThrows)));
Assert.NotNull(exception.StackTrace);

await Assert.ThrowsAnyAsync<ObjectDisposedException>(() => this.clientRpc.InvokeAsync<string>(nameof(Server.AsyncMethodThatThrows)));
}

[Fact]
public async Task ThrowsIfCannotFindMethod_Overload()
{
await Assert.ThrowsAsync(typeof(RemoteMethodNotFoundException), () => this.clientRpc.InvokeAsync("missingMethod", 50));

var result = await this.clientRpc.InvokeAsync<string>(nameof(Server.ServerMethod), "testing");
Assert.Equal("testing!", result);
}

// Covers bug https://github.com/Microsoft/vs-streamjsonrpc/issues/55
// Covers bug https://github.com/Microsoft/vs-streamjsonrpc/issues/56
[Fact]
public async Task InvokeWithCancellationAsync_CancelOnFirstWriteToStream()
{
// TODO: remove the next line when https://github.com/Microsoft/vs-threading/issues/185 is fixed
this.server.DelayAsyncMethodWithCancellation = true;

// Repeat 10 times because https://github.com/Microsoft/vs-streamjsonrpc/issues/56 is a timing issue and we may miss it on the first attempt.
for (int iteration = 0; iteration < 10; iteration++)
Copy link
Member

Choose a reason for hiding this comment

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

Does repeating this test 10 times guarantee a hit for that bug?

{
using (var cts = new CancellationTokenSource())
{
this.clientStream.BeforeWrite = (stream, buffer, offset, count) =>
{
// Cancel on the first write, when the header is being written but the content is not yet.
Copy link
Member

Choose a reason for hiding this comment

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

content is not yet? Is it me or does this sound like it's missing something or requires a different phrasing?

if (!cts.IsCancellationRequested)
{
cts.Cancel();
}
};

await Assert.ThrowsAsync<TaskCanceledException>(() => this.clientRpc.InvokeWithCancellationAsync<string>(nameof(Server.AsyncMethodWithCancellation), new[] { "a" }, cts.Token)).WithTimeout(UnexpectedTimeout);
this.clientStream.BeforeWrite = null;
}

// Verify that json rpc is still operational after cancellation.
// If the cancellation breaks the json rpc, like in https://github.com/Microsoft/vs-streamjsonrpc/issues/55, it will close the stream
// and cancel the request, resulting in unexpected OperationCancelledException thrown from the next InvokeAsync
string result = await this.clientRpc.InvokeAsync<string>(nameof(Server.ServerMethod), "a");
Assert.Equal("a!", result);
}
}

[Fact]
public async Task CancelMessageSentWhileAwaitingResponse()
{
using (var cts = new CancellationTokenSource())
{
var invokeTask = this.clientRpc.InvokeWithCancellationAsync<string>(nameof(Server.AsyncMethodWithCancellation), new[] { "a" }, cts.Token);
await this.server.ServerMethodReached.WaitAsync(this.TimeoutToken);
cts.Cancel();

// Ultimately, the server throws because it was canceled.
var ex = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => invokeTask.WithTimeout(UnexpectedTimeout));
#if !NET452
Assert.Equal(cts.Token, ex.CancellationToken);
#endif
}

var result = await this.clientRpc.InvokeAsync<string>(nameof(Server.ServerMethod), "testing");
Assert.Equal("testing!", result);
}

[Fact]
public async Task InvokeWithCancellationAsync_CanCallCancellableMethodWithNoArgs()
{
Assert.Equal(5, await this.clientRpc.InvokeWithCancellationAsync<int>(nameof(Server.AsyncMethodWithCancellationAndNoArgs)));

using (var cts = new CancellationTokenSource())
{
Task<int> resultTask = this.clientRpc.InvokeWithCancellationAsync<int>(nameof(Server.AsyncMethodWithCancellationAndNoArgs), cancellationToken: cts.Token);
cts.Cancel();
try
{
int result = await resultTask;
Assert.Equal(5, result);
}
catch (OperationCanceledException)
{
// this is also an acceptable result.
Copy link
Member

Choose a reason for hiding this comment

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

If this is the exception sent by the server (iirc how this works), should we check for the integrity of the exception? Making sure it has the correct content, stack trace, and any other information that would be helpful to the user.

}
}
}

[Fact]
public async Task CancelMayStillReturnErrorFromServer()
{
using (var cts = new CancellationTokenSource())
{
var invokeTask = this.clientRpc.InvokeWithCancellationAsync<string>(nameof(Server.AsyncMethodFaultsAfterCancellation), new[] { "a" }, cts.Token);
await this.server.ServerMethodReached.WaitAsync(this.TimeoutToken);
cts.Cancel();
this.server.AllowServerMethodToReturn.Set();

await Assert.ThrowsAsync<TaskCanceledException>(() => invokeTask);
}
}

[Fact]
public async Task AsyncMethodThrows()
{
await Assert.ThrowsAsync<TaskCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.MethodThatThrowsAsync)));
}

public class Server
{
internal const string ThrowAfterCancellationMessage = "Throw after cancellation";

public bool DelayAsyncMethodWithCancellation { get; set; }

public AsyncAutoResetEvent ServerMethodReached { get; } = new AsyncAutoResetEvent();

public AsyncAutoResetEvent AllowServerMethodToReturn { get; } = new AsyncAutoResetEvent();

public static string ServerMethod(string argument)
{
return argument + "!";
}

public Task ServerMethodThatReturnsCancelledTask()
{
var tcs = new TaskCompletionSource<object>();
tcs.SetCanceled();
return tcs.Task;
}

public void MethodThatThrowsUnauthorizedAccessException()
{
throw new UnauthorizedAccessException();
}

public async Task AsyncMethodThatThrows()
{
await Task.Yield();
throw new Exception();
}

public async Task<string> AsyncMethodWithCancellation(string arg, CancellationToken cancellationToken)
{
this.ServerMethodReached.Set();

// TODO: remove when https://github.com/Microsoft/vs-threading/issues/185 is fixed
if (this.DelayAsyncMethodWithCancellation)
{
await Task.Delay(UnexpectedTimeout).WithCancellation(cancellationToken);
}

await this.AllowServerMethodToReturn.WaitAsync(cancellationToken);
return arg + "!";
}

public async Task<int> AsyncMethodWithCancellationAndNoArgs(CancellationToken cancellationToken)
{
await Task.Yield();
return 5;
}

public async Task<string> AsyncMethodFaultsAfterCancellation(string arg, CancellationToken cancellationToken)
{
this.ServerMethodReached.Set();
await this.AllowServerMethodToReturn.WaitAsync();
if (!cancellationToken.IsCancellationRequested)
{
var cancellationSignal = new AsyncManualResetEvent();
using (cancellationToken.Register(() => cancellationSignal.Set()))
{
await cancellationSignal;
}
}

throw new InvalidOperationException(ThrowAfterCancellationMessage);
}

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

internal class JsonRpcOverload : JsonRpc
Copy link
Member

Choose a reason for hiding this comment

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

The term 'overload' is usually used with regard to multiple methods with the same name but different parameters. For derived types, a name that describes how it is different seems more appropriate. For example: JsonRpcWithFatalExceptions.

Then perhaps renaming this test class to use something like that name instead of the overload term.

{
public JsonRpcOverload(Stream sendingStream, Stream receivingStream, object target = null)
: base(sendingStream, receivingStream, target)
{
}

protected override bool IsFatalException(Exception ex) => true;
}
}
5 changes: 5 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcTests.cs
Expand Up @@ -165,6 +165,11 @@ public async Task CanPassExceptionFromServer()
RemoteInvocationException exception = await Assert.ThrowsAnyAsync<RemoteInvocationException>(() => this.clientRpc.InvokeAsync(nameof(Server.MethodThatThrowsUnauthorizedAccessException)));
Assert.NotNull(exception.RemoteStackTrace);
Assert.StrictEqual(COR_E_UNAUTHORIZEDACCESS.ToString(CultureInfo.InvariantCulture), exception.RemoteErrorCode);

var result = await this.clientRpc.InvokeAsync<Foo>(nameof(Server.MethodThatAcceptsFoo), new { Bar = "bar", Bazz = 1000 });
Assert.NotNull(result);
Assert.Equal("bar!", result.Bar);
Assert.Equal(1001, result.Bazz);
Copy link
Member

Choose a reason for hiding this comment

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

This added segment does not appear to be related to what the test method was previously testing, and doesn't seem to fit under the test method name. Why does it belong here? Should it be moved to its own method, or do we even need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I missed that. I added this check a while ago when I was first starting to look into the JsonRpc object being disposed/not disposed.

}

[Fact]
Expand Down
12 changes: 10 additions & 2 deletions src/StreamJsonRpc/JsonRpc.cs
Expand Up @@ -11,6 +11,7 @@ namespace StreamJsonRpc
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.ExceptionServices;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -612,6 +613,13 @@ protected virtual void Dispose(bool disposing)
}
}

/// <summary>
/// Indicates whether the connection should be closed if the server throws an exception.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "when the server throws an exception" rather than "if the server throws an exception" IMO helps convey the idea that the method is invoked in the moment of the exception rather than at startup or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

/// </summary>
/// <param name="ex">The <see cref="Exception"/> thrown from server that is potentially fatal</param>
/// <returns>A <see cref="bool"/> indicating if the streams should be closed.</returns>
Copy link
Member

Choose a reason for hiding this comment

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

I think a <remarks> section here seems appropriate. It would likely cover:

  1. Suggesting the use of Environment.FailFast if the process should crash is appropriate here.
  2. That this method is invoked within the context of an exception filter may be useful to some folks.
  3. That the default implementation is to simply return false.
  4. That an override that returns true should probably take care to return false to certain expected exceptions such as OperationCanceledException.

protected virtual bool IsFatalException(Exception ex) => false;

/// <summary>
/// Invokes the specified RPC method
/// </summary>
Expand Down Expand Up @@ -916,7 +924,7 @@ private async Task<JsonRpcMessage> DispatchIncomingRequestAsync(JsonRpcMessage r

return await ((Task)result).ContinueWith(this.handleInvocationTaskResultDelegate, request.Id, TaskScheduler.Default).ConfigureAwait(false);
}
catch (Exception ex)
catch (Exception ex) when (!this.IsFatalException(ex))
{
return CreateError(request.Id, ex);
}
Expand Down Expand Up @@ -944,7 +952,7 @@ private JsonRpcMessage HandleInvocationTaskResult(JToken id, Task t)
throw new ArgumentException(Resources.TaskNotCompleted, nameof(t));
}

if (t.IsFaulted)
if (t.IsFaulted && !this.IsFatalException(t.Exception))
Copy link
Member

Choose a reason for hiding this comment

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

After this line in this method, we call Task<T>.Result if it's a Task<T>. This will throw an AggregateException which is probably not the best exception to throw (we'd rather throw the AggregateException.InnerException).
If t is just Task, we don't call it at all so this method won't throw.
I would suggest you add tests to verify that the exception thrown is of the expected type in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the final exception that's surfaced to the client is always TaskCanceledException, but it's a good catch. I've updated it and added tests to ensure the exception that's passed through the filter is at least what's expected in either case. Thanks!

{
return CreateError(id, t.Exception);
}
Expand Down
1 change: 1 addition & 0 deletions src/StreamJsonRpc/PublicAPI.Shipped.txt
Expand Up @@ -68,6 +68,7 @@ static StreamJsonRpc.JsonRpc.Attach(System.IO.Stream stream, object target = nul
virtual StreamJsonRpc.DelimitedMessageHandler.Dispose(bool disposing) -> void
virtual StreamJsonRpc.JsonRpc.Dispose(bool disposing) -> void
virtual StreamJsonRpc.JsonRpc.InvokeCoreAsync<TResult>(int? id, string targetName, System.Collections.Generic.IReadOnlyList<object> arguments, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task<TResult>
virtual StreamJsonRpc.JsonRpc.IsFatalException(Exception ex) -> bool
StreamJsonRpc.JsonRpc.NotifyAsync(string targetName, object argument) -> System.Threading.Tasks.Task
StreamJsonRpc.JsonRpc.InvokeAsync(string targetName, object argument) -> System.Threading.Tasks.Task
StreamJsonRpc.JsonRpc.InvokeAsync<TResult>(string targetName, object argument) -> System.Threading.Tasks.Task<TResult>
Expand Down