Allow server the option to crash on exception thrown #74

jepetty · 2017-12-15T00:19:43Z

Fixes #69

AArnott · 2017-12-15T21:05:27Z

src/StreamJsonRpc/JsonRpc.cs

@@ -77,6 +77,7 @@ public partial class JsonRpc : IDisposableObservable
        private bool disposed;
        private bool hasDisconnectedEventBeenRaised;
        private bool startedListening;
+        private bool closeStreamOnException = false;


FxCop: Avoid field initializers that set the field to their default value. more info

Also, this field doesn't appear to be set anywhere else, which makes me wonder why the field exists.

AArnott · 2017-12-15T22:18:24Z

src/StreamJsonRpc/JsonRpc.cs

+        /// Indicates whether the connection should be closed if the server throws an exception.
+        /// </summary>
+        /// <returns>A <see cref="bool"/> indicating if the streams should be closed.</returns>
+        protected virtual bool ShouldCloseStreamOnException() => this.closeStreamOnException;


I'm thinking the implementation might want to make its decision based on the Exception object. Can we pass that in as a parameter?

AArnott · 2017-12-15T22:22:43Z

src/StreamJsonRpc/JsonRpc.cs

+                    throw t.Exception.InnerException;
+                }
+
+                throw t.Exception;


Rethrowing an Exception like this stomps on its Stacktrace property with a new one, blowing away the evidence of the original one. Instead, rethrowing should be done with ExceptionDispatchInfo.Capture(t).Throw().

AArnott · 2017-12-15T22:26:57Z

src/StreamJsonRpc/JsonRpc.cs

            {
                return CreateError(id, t.Exception);
            }

+            if (t.IsFaulted && this.ShouldCloseStreamOnException())
+            {
+                if (t.Exception is AggregateException && t.Exception.InnerException != null)


Task.Exception is always an AggregateException (it's actually typed as such). So you can drop that part of the check.

Then your comment mentions you want to strip off the TargetInvocationException but in fact it appears you're not, since you're only removing the AggregateException.

AArnott · 2017-12-15T22:28:49Z

src/StreamJsonRpc.Tests/JsonRpcTests.cs

+    {
+        var streams = FullDuplexStream.CreateStreams();
+        var overloadRpc = new JsonRpcCrashesOnException(streams.Item1, streams.Item2, new Server());
+        overloadRpc.StartListening();


Given the number of tests you added and the proportion of each one containing this boilerplate code, IMO this should be moved into its own test class with a constructor that contains the boilerplate code.

Also, it appears you've doubled up on a lot of existing test methods. If it's necessary to achieve double-coverage for the different configuration, let's do that in a way that doesn't require maintaining double copies of every test. But I'm hoping that we only need to double-up on the tests that actually include the server throwing exceptions.
BTW, I don't see any code here to handle or suppress crashing. Where is it?

jepetty · 2017-12-19T00:43:09Z

It seems to me that if the user decides to callEnvironment.FailFast from their overloaded IsFatalException method:

Stack data will be preserved because crash will occur at basically the same time we would have crashed
They won't need to wrap all of their InvokeAsync calls with try/catch
We won't be responsible for crashes because we won't be crashing 😃

javierdlg · 2017-12-19T14:12:05Z

src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs

+        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++)


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

javierdlg · 2017-12-19T14:13:36Z

src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs

+            {
+                this.clientStream.BeforeWrite = (stream, buffer, offset, count) =>
+                {
+                    // Cancel on the first write, when the header is being written but the content is not yet.


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

javierdlg · 2017-12-19T14:16:45Z

src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs

+            }
+            catch (OperationCanceledException)
+            {
+                // this is also an acceptable result.


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.

AArnott

Looking better. The product changes look pretty good. The tests need a few touch-ups.

AArnott · 2017-12-19T16:30:04Z

README.md

@@ -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/


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?

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?

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

AArnott · 2017-12-19T16:31:51Z

src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs

+        }
+    }
+
+    internal class JsonRpcOverload : JsonRpc


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.

AArnott · 2017-12-19T16:33:04Z

src/StreamJsonRpc.Tests/JsonRpcTests.cs

+        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);


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?

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.

AArnott · 2017-12-19T16:36:12Z

src/StreamJsonRpc/JsonRpc.cs

@@ -612,6 +613,13 @@ protected virtual void Dispose(bool disposing)
            }
        }

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


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.

AArnott · 2017-12-19T16:37:30Z

src/StreamJsonRpc/JsonRpc.cs

+        /// Indicates whether the connection should be closed if the server throws an exception.
+        /// </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>


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

Suggesting the use of Environment.FailFast if the process should crash is appropriate here.

That this method is invoked within the context of an exception filter may be useful to some folks.

That the default implementation is to simply return false.

That an override that returns true should probably take care to return false to certain expected exceptions such as OperationCanceledException.

AArnott · 2017-12-19T16:45:13Z

src/StreamJsonRpc/JsonRpc.cs

@@ -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))


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.

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!

AArnott · 2017-12-19T17:26:18Z

src/StreamJsonRpc.Tests/JsonRpcOverloadTests.cs

+using Xunit.Abstractions;
+using static JsonRpcMethodAttributeTests;
+
+public class JsonRpcOverloadTests : TestBase


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.

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.

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.

jepetty · 2017-12-20T01:23:36Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        var exceptionMessage = "Exception from CloseStreamsOnSynchronousMethodException";
+        OperationCanceledException exception = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => this.clientRpc.InvokeAsync(nameof(Server.MethodThatThrowsUnauthorizedAccessException), exceptionMessage));
+        Assert.NotNull(exception.StackTrace);
+        Assert.Equal(FaultException.InnerException.Message, exceptionMessage);


The exception that's passed into the filter is TargetInvocationException in the synchronous case and AggregateException in the asynchronous case. Would you rather I document that, or use the inner exception in the filter?
(I'm leaning towards documenting, myself)

I'm leaning toward sharing the original exception rather than the exception we add as an artifact of how we invoke the method.
For AggregateException, is that thrown because Task.Exception is thrown? What is the InnerException? Is it a TargetInvocationException?

I'd really like to get to a consistent exception type because I suspect folks who override this method will not anticipate that async vs. sync methods will need to be tested separately. If they're all TargetInvocationException or AggregateException that might be OK, but the mix of the two sounds like it's a recipe for overrides to be buggy. I'm guessing their first guess (at least in lieu of docs) would be that if they throw ArgumentException from their server method, that they'll see an ArgumentException in the exception filter. And IMO that seems like a reasonable goal for us, if we can reach it.

…on or AggregateException

AArnott

I haven't fully reviewed all your latest changes yet, but I have to run so here's what I have so far.

AArnott · 2017-12-20T17:31:06Z

doc/index.md

+
+    protected override bool IsFatalException(Exception ex)
+    {
+        if (ex.GetType() != typeof(OperationCanceledException))


Prefer ex is OperationCanceledException over explicitly calling GetType() and typeof. In your case you'd need to flip the logic though, so either !(ex is OperationCanceledException) or flip the blocks around.

AArnott · 2017-12-20T17:32:39Z

doc/index.md

+    {
+        if (ex.GetType() != typeof(OperationCanceledException))
+        {
+            Environment.FailFast(ex.message);


You definitely want to call the FailFast overload that accepts ex as a second parameter.

AArnott · 2017-12-20T17:33:49Z

doc/index.md

+```
+
+## Crashing the process on exception
+In some cases, you may want to immediately crash the server and client processes if certain exceptions are thrown. In this case, overriding the `IsFatalException` method will give you the desired functionality. `IsFatalException` provides an exception filter through which you can access and respond to exceptions as they are caught.


"...and client processes..."

I don't think that's the scenario we're designing for, nor what you demonstrate below. It's just the server that crashes in this design, right?

AArnott · 2017-12-20T17:35:55Z

doc/index.md

+    public async Task NotifyRemote()
+    {
+        var target = new Server();
+        var rpc = new JsonRpcCrashesOnException(Console.OpenStandardOutput(), Console.OpenStandardInput(), target);


I'm thoroughly confused by this. It appears you're working with console streams, which is a little odd in the first place. But then you're using the same instance of JsonRpc both to invoke the server method as a client and also to invoke it as the server itself. I'm pretty sure that doesn't work.

I'm mostly trying to stay consistent with the rest of the documentation (although I see another example of using the constructor directly uses FullDuplexStream). I do agree with your confusion though; these examples confused me when I first started looking into this and I got more value from looking at the unit tests.

Ok. I didn't thoroughly review the docs we have right now, which came from Tina. So feel free to improve on them in your addition, and possibly fix up the originals too. :)

AArnott · 2017-12-20T17:36:37Z

src/StreamJsonRpc/JsonRpc.cs

+        /// This method is invoked within the context of an exception filter and simply returns false by default.
+        /// Certain exceptions (such as <see cref="OperationCanceledException"/>) are not fatal in most cases, therefore
+        /// care should be taken to return false and keep the connection open. If the process should crash on an exception,
+        /// calling Environment.FailFast will produce such behavior.


Environment.FailFast -> <see cref="Environment.FailFast(string, Exception)" />

AArnott · 2017-12-20T17:39:53Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+
+public class JsonRpcWithFatalExceptionsTests : TestBase
+{
+    internal static Exception FaultException;


xunit can run test methods in parallel, so you should avoid statics to avoid unstable tests.
Instead, you can leave this as an instance property on your nested class since that's where it's determined anyway, and then your test methods can access that property since they instantiated the nested class in the first place.

I like it! Thanks!

AArnott · 2017-12-20T17:52:47Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        this.clientStream = streams.Item2;
+
+        this.messageHandler = new DisposingMessageHander(this.clientStream, this.serverStream);
+        this.clientRpc = new JsonRpcWithFatalExceptions(this.messageHandler, this.server);


If you look at the JsonRpcTests class, you'll see we create instantiate JsonRpc twice: once for the client and once for the server. In this case, you appear to be hooking up the "server" to the clientRpc object, which looks wrong, and I can't see a second instance being created. So I imagine when you call this.clientRpc.Invoke it will send it to the serverStream but with no one listening to it, it will just hang there.

Sounds good! I don't really know best practices, but there wasn't a hang with this implementation. When I update the docs I'll make sure they instantiate two instances like we do in the tests, as well.

Interesting. That suggests that you've successfully formed a loopback connection with JsonRpc. I've never seen that 😄 .
If it worked, I probably wouldn't mind if you kept it that way, but using two JsonRpc instances has the advantage of resembling how customers will likely use it more closely, which increases our confidence that we're testing the right thing.

I still agree we should change it! Especially since there will most likely be people like me who look to our unit tests for added documentation.

AArnott · 2017-12-20T17:58:07Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        }
+    }
+
+    public class DisposingDuplexStream : FullDuplexStream


If all you need is an IsDisposed property, you could just add that to the FullDuplexStream class defined within this project. It seems like it would be less work and possibly be useful to other tests in this project.

AArnott · 2017-12-20T18:01:27Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        // Assert MessageHandler and Stream objects are not disposed
+        Assert.False(((DisposingMessageHander)this.clientRpc.MessageHandler).IsDisposed);
+        Assert.False(this.serverStream.IsDisposed);
+        Assert.False(this.clientStream.IsDisposed);


Verifying both the message handler and the streams seems overkill, IMO. Particularly since you're actually controlling the MessageHandler being used. I would probably just test MessageHandler's disposal. And then confirm that our message handler tests verify that when the MessageHandler is disposed it disposes of the streams.

AArnott · 2017-12-20T18:03:01Z

src/StreamJsonRpc/JsonRpc.cs

+                    {
+                        ExceptionDispatchInfo.Capture(t.Exception.InnerException).Throw();
+                    }
+                    else if (this.IsFatalException(t.Exception))


This seems odd that you're calling IsFatalException twice for ultimately the same exception. Can we get it to just be called once? (and then perhaps add assertions to your tests to ensure it's only invoked once so it doesn't regress).

AArnott · 2017-12-20T18:06:31Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        public async Task AsyncMethodThatThrowsBeforeYield(string message)
+        {
+            throw new Exception(message);
+            await Task.Yield();


If you get build warnings from dead code on this line, you can replace this method with:

public Task AsyncMethodThatThrowsBeforeYield(string message) { var tcs = new TaskCompletionSource<object>(); tcs.SetException(new Exception(message)); return tcs.Task; }

Awesome, thanks!

AArnott · 2017-12-20T18:10:28Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        }
+
+        // Assert MessageHandler and Stream objects are not disposed
+        Assert.False(((DisposingMessageHander)this.clientRpc.MessageHandler).IsDisposed);


Why shouldn't they be disposed on cancellation? Your IsFatalException override returns true unconditionally. So I read that to mean that TaskCanceledException is fatal.

Good point. I was thinking we only needed to worry about exceptions thrown explicitly from the server method (but this would miss the whole gambit of OperationCanceledExceptions. Fixed in my next iteration

I was thinking we only needed to worry about exceptions thrown explicitly from the server method

I would agree -- but when a server method is canceled, it will in fact throw this exception -- unless perhaps the request to invoke is canceled before we even invoke the method to begin with, in which case the exception might never be thrown, theoretically. And in that case, I guess I'd be ok with us not calling IsFatalException at all in that case since indeed the server method didn't throw it.

Regardless, I was missing the case where the server explicitly throws an OperationCanceledException in an async method, so it was a great catch!

AArnott · 2017-12-20T18:11:18Z

src/StreamJsonRpc/JsonRpc.cs

+        /// <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>
+        /// <remarks>
+        /// This method is invoked within the context of an exception filter and simply returns false by default.


I guess we shouldn't say it's called from an exception filter since that is not always the case. I see from your other change that we only call it in an exception filter in one of a few cases.

AArnott · 2017-12-20T18:11:46Z

doc/index.md

+```
+
+## Crashing the process on exception
+In some cases, you may want to immediately crash the server and client processes if certain exceptions are thrown. In this case, overriding the `IsFatalException` method will give you the desired functionality. `IsFatalException` provides an exception filter through which you can access and respond to exceptions as they are caught.


We may want to remove the "exception filter" reference since it is not always called from an exception filter.

…ssageHandler tests

AArnott · 2018-01-04T21:44:36Z

doc/index.md

-    var rpc = JsonRpc.Attach(Console.OpenStandardOutput(), Console.OpenStandardInput(), target);
+    var streams = Nerdbank.FullDuplexStream.CreateStreams();
+    var clientRpc = JsonRpc.Attach(streams.Item1);
+    var serverRpc = JsonRpc.Attach(streams.Item2, target);


In each of your updated samples (thanks for fixing them, BTW) I would focus it on either the client or the server side rather than show both in the same method. And it seems quite acceptable to use syntax that assumes the stream is handed to you rather than construct one or use the Console stream.
Reserve the FullDuplexStream sample for a "how to test this?" section, if we have one.

AArnott · 2018-01-04T21:47:43Z

doc/index.md

+}
+```
+
+## Crashing the process on exception


Crashing may not be the best thing to advertise. It also leads to a sample where you return false or throw -- never returning true, which begs the question why we made it a bool returning method.
Instead, how about we document this as a "Close stream on fatal errors" and document as returning true or false. Then as a comment we can mention that Environment.FailFast might be used here to crash the entire process instead -- but I'm on the fence about whether we should even mention that.

AArnott · 2018-01-04T21:56:58Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

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


This leaves open a race condition that allows for MethodThatThrowsAsync to return a completed task or an incomplete task. To ensure it is never completed, rewrite the body of this method to be:

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

I've already implemented that in AsyncMethodThatThrowsAfterYield. I'll remove this method and it's corresponding tests since it doesn't seem to be doing anything unique.

AArnott · 2018-01-04T21:58:47Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+
+        public async Task<string> AsyncMethodThatReturnsStringAndThrows(string message)
+        {
+            await Task.Run(() => throw new Exception(message));


Rewrite to this to guarantee the method is always asynchronous.

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

AArnott · 2018-01-04T22:04:04Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

+        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);


I'd like to chat to better understand why this is 2 instead of just 1.

When I rethrew the exception we encountered it twice: once within the task handler and then once again when it was thrown from the task handler. I've changed it to close the streams within the task handler so there's no need to see the exception again.

AArnott · 2018-01-04T22:06:38Z

src/StreamJsonRpc/JsonRpc.cs

@@ -916,7 +930,7 @@ private static JsonRpcMessage CreateError(JToken id, Exception exception)

                return await ((Task)result).ContinueWith(this.handleInvocationTaskResultDelegate, request.Id, TaskScheduler.Default).ConfigureAwait(false);
            }
-            catch (Exception ex)
+            catch (Exception ex) when (!this.IsFatalException(ex.InnerException ?? ex))


This redirection to the InnerException if it's defined makes me nervous that we might disregard an outer exception that is useful. I expect you're trying to strip off the TargetInvocationException and/or AggregateException, is that right? In that case, can we add new catch blocks above this one for those specific types and strip the outer exception just for those, leaving any other exceptions alone?

Or perhaps since you want to strip it in other places as well that don't use catch blocks, you can write a helper method that does the type checking on the exception and strips it only when it's appropriate.

AArnott · 2018-01-04T22:07:58Z

src/StreamJsonRpc/JsonRpc.cs

@@ -946,11 +960,34 @@ private JsonRpcMessage HandleInvocationTaskResult(JToken id, Task t)

            if (t.IsFaulted)
            {
+                if (t.Exception != null)


This is something you can assume without checking: Task.IsFaulted === (Task.Exception != null) is a guarantee.

AArnott · 2018-01-04T22:10:34Z

src/StreamJsonRpc/JsonRpc.cs

                return CreateError(id, t.Exception);
            }

            if (t.IsCanceled)
            {
+                if (this.IsFatalException(new OperationCanceledException()))


I don't think we should manufacture an exception here. If the method we invoked doesn't actually throw an exception, but rather returns a Canceled task, that likely means that the server method went out of its way to skip an exception code path and we shouldn't artificially bring it back. Anyway, the method is called IsFatalException and no exception occurred.

I found that any time an asynchronous task was canceled (or if the server method threw OperationCanceledException) it ended up in this handler as a canceled task. I agree though, this felt weird, and especially with there being no way to distinguish between an exception thrown or a canceled task returned. Still better to remove it and document that they don't need to filter for TaskCanceledException after all?

AArnott · 2018-01-10T17:24:00Z

doc/index.md

-        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");


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..."

AArnott · 2018-01-10T17:27:06Z

doc/index.md

-## 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.


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.

AArnott · 2018-01-10T17:28:37Z

src/StreamJsonRpc.Tests/JsonRpcWithFatalExceptionsTests.cs

@@ -219,7 +204,7 @@ public Task AsyncMethodThatThrowsBeforeYield(string message)
        {
            await Task.Run(() => throw new Exception(message));

-            return "never will return";
+            return await Task.FromResult("never will return");


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.

AArnott · 2018-01-10T17:31:08Z

src/StreamJsonRpc/EventArgs/DisconnectedReason.cs

@@ -27,5 +27,10 @@ public enum DisconnectedReason
        /// The stream was disposed.
        /// </summary>
        Disposed,
+
+        /// <summary>
+        /// A fatal exception was thrown from the server method.


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.

AArnott · 2018-01-10T17:32:11Z

src/StreamJsonRpc/JsonRpc.cs

+        }
+
+        private static Exception StripExceptionToInnerException(Exception exception)
+        {
            if (exception is TargetInvocationException || (exception is AggregateException && exception.InnerException != null))


This helper method looks much better. I'm a little concerned about inconsistent handling of AggregateException, which we can talk about in person since I find it hard to explain without illustrations.

Maybe I'll just push a couple new test methods to your branch to demonstrate what I believe may be a problem.

AArnott · 2018-01-10T17:36:29Z

src/StreamJsonRpc/Resources.resx

@@ -211,7 +211,7 @@
    <comment>An error message when a stream passed in as an argument is incorrect.</comment>
  </data>
  <data name="SumOfTwoParametersExceedsArrayLength" xml:space="preserve">
-    <value>The {0] and {1} parameters exceed length of array.</value>
+    <value>The {0 and {1} parameters exceed length of array.</value>


It appears you have a formatting string bug here.

AArnott · 2018-01-10T17:37:22Z

src/StreamJsonRpc/JsonRpc.cs

+                    var exception = StripExceptionToInnerException(t.Exception);
+                    var e = new JsonRpcDisconnectedEventArgs(
+                        string.Format(CultureInfo.CurrentCulture, Resources.FatalExceptionWasThrown, exception.GetType(), exception.Message),
+                        DisconnectedReason.Unknown,


Why are we using Unknown as the reason when you've defined a new enum value specifically for this situation?

…o the filter any more

jepetty added 3 commits December 14, 2017 15:34

update readme, allow server to crash on exception

de75371

remove temp exception in favor of taskCanceledException

43d9c91

revert change in statement order

d5f7435

AArnott requested changes Dec 15, 2017

View reviewed changes

jepetty added 4 commits December 15, 2017 15:41

resolve merge conflicts

226db0a

separate overloaded rpc tests and remove unnecessary tests

0bbc033

update API to include exception as parameter

673d727

add parameter documentation

ef74c1a

javierdlg reviewed Dec 19, 2017

View reviewed changes

AArnott requested changes Dec 19, 2017

View reviewed changes

jepetty added 2 commits December 19, 2017 16:56

update documentation for new functionality

8b9678c

improve tests and add remarks

8152cf4

jepetty commented Dec 20, 2017

View reviewed changes

Only pass inner exception through filter, not TargetInvocationExcepti…

3b1efda

…on or AggregateException

AArnott requested changes Dec 20, 2017

View reviewed changes

AArnott reviewed Dec 20, 2017

View reviewed changes

AArnott requested changes Dec 20, 2017

View reviewed changes

jepetty added 6 commits December 20, 2017 10:18

update docs as per cr feedback

5994ffb

update all parts of doc to instantiate JsonRpc twice

747af41

closing streams on operation canceled, other pr comments

5435f72

Move IsDisposed to FullDuplexStreams and check streams disposed in Me…

97f2c56

…ssageHandler tests

fix warnings in error list

89a602e

fix build error

dbde851

AArnott requested changes Jan 4, 2018

View reviewed changes

AArnott added this to the v1.4 milestone Jan 6, 2018

update documentation, only filter exceptions once

3d04487

AArnott requested changes Jan 10, 2018

View reviewed changes

AArnott added 2 commits January 10, 2018 09:58

Remove obsolete comment since we don't send cancellation exceptions t…

12c6c54

…o the filter any more

Add a few tests around AggregateException

22a1a68

jepetty added 3 commits January 10, 2018 17:45

responding to pr feedback

a0477dd

merge master branch

ae643fa

fix resx file tag, make test always async

984c20c

AArnott approved these changes Jan 12, 2018

View reviewed changes

jepetty merged commit 344cdfc into master Jan 12, 2018

AArnott deleted the dev/jepetty/issue69 branch January 12, 2018 19:23

Allow server the option to crash on exception thrown #74

Allow server the option to crash on exception thrown #74

Conversation

jepetty commented Dec 15, 2017 • edited by AArnott

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jepetty commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AArnott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AArnott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jepetty Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jepetty commented Dec 15, 2017 •

edited by AArnott

jepetty Jan 8, 2018 •

edited