Implement ability to add remote RPC targets #319

tinaschrepfer · 2019-08-13T20:06:08Z

Implementation according to design review.

Merge from main master

Merge from master

AArnott

I've nit-picked at a few things, but I can't remember the design discussion we had so it would be helpful it you'd edit the PR description to give the high level objective, as well as xml doc comments to educate users (and reviewers). :)

AArnott · 2019-08-13T22:09:49Z

src/StreamJsonRpc/JsonRpc.cs

@@ -577,6 +579,13 @@ public T Attach<T>(JsonRpcProxyOptions options)
            return proxy;
        }

+        public void Relay(Stream relaySendingStream, Stream relayReceivingStream, object relayTarget = null)
+        {
+            this.relayRpc = JsonRpc.Attach(relaySendingStream, relayReceivingStream, relayTarget);


Setting the field even if it was set previously sounds like it would be an error. Should we throw InvalidOperationException when it has already been set? (tip: add a test for this).

Yup, I haven't gone back and polished the PR. Just wanted to get a read from you initially to see if this is the right approach.

AArnott · 2019-08-13T22:10:19Z

src/StreamJsonRpc/JsonRpc.cs

+        {
+            this.relayRpc = JsonRpc.Attach(relaySendingStream, relayReceivingStream, relayTarget);
+
+            this.relayRpc.relayRpc = this;


This appears to cause a circular loop between the two JsonRpc instances in which the original one is imperceptible from the one created here. Is that right (and good)?

AArnott · 2019-08-13T22:11:13Z

src/StreamJsonRpc/JsonRpc.cs

+        protected async Task<TResult> InvokeCoreAsync<TResult>(JsonRpcRequest request, CancellationToken cancellationToken)
+        {
+            long id = default(long);
+            if (request.Id is long)


don't dereference arguments without checking for null first.

AArnott · 2019-08-13T22:38:40Z

src/StreamJsonRpc/JsonRpc.cs

+
+        protected async Task<TResult> InvokeCoreAsync<TResult>(JsonRpcRequest request, CancellationToken cancellationToken)
+        {
+            long id = default(long);


In the event that request.Id is null, this will leave id=0, which is not the same thing as null. Being null implies a notification while 0 is a request that expects a response.
Also, in the general relaying configuration, you can't assume that the request.Id is even a number. It's allowed to be string, and some JSON-RPC servers do that. So if we're relaying messages from non-StreamJsonRpc sources, we'd need to properly handle all legal values for id.

AArnott · 2019-08-13T22:42:46Z

src/StreamJsonRpc/JsonRpc.cs

+            return await this.InvokeCoreAsync<TResult>(request, cancellationToken).ConfigureAwait(false);
+        }
+
+        protected async Task<TResult> InvokeCoreAsync<TResult>(JsonRpcRequest request, CancellationToken cancellationToken)


Does this overload need to be protected or can it be private?

It can probably be private. I originally had a subclass of JsonRpc, but found that was not necessary.

AArnott · 2019-08-13T22:48:31Z

src/StreamJsonRpc/JsonRpc.cs

@@ -577,6 +579,13 @@ public T Attach<T>(JsonRpcProxyOptions options)
            return proxy;
        }

+        public void Relay(Stream relaySendingStream, Stream relayReceivingStream, object relayTarget = null)


Please add detailed xml doc comments to this method, including a remarks section that explains the scenarios that it is meant to work with.

…eamjsonrpc into relayStream

AArnott · 2019-08-26T20:38:18Z

src/StreamJsonRpc/JsonRpc.cs

@@ -675,6 +681,18 @@ public void AddLocalRpcTarget(object target, JsonRpcTargetOptions options)
            }
        }

+        /// <summary>
+        /// Adds a remote rpc connection so calls can be forwarded to the remote target if local targets do not handle it.


You support remote being a fallback to local handlers. Do you need or want to support local being a fallback to remote? I'm guessing not. But just calling it out.

Local fallback would always take precedence. For example, if both local target and remote target has the method "Foo()", then the local one would be invoked before we even check for remote.

AArnott · 2019-08-26T20:49:25Z

src/StreamJsonRpc.Tests/JsonRpcRemoteTargetTests.cs

+        remoteTarget1.AllowModificationWhileListening = true;
+
+        var remoteTarget2 = JsonRpc.Attach(remoteClientStream2, remoteClientStream2, new LocalRelayTarget());
+        remoteTarget2.AllowModificationWhileListening = true;


Is there any reason to be concerned that this is required? Generally, modifying the configuration after listening has started is a bad idea. Tests are sometimes used as samples (especially early on) so it's generally preferable to not use this property in a test unless you're explicitly testing that setting this property to true allows configuration changes.
The preferred pattern for fancy configurations is to use the constructor, add targets, then start listening explicitly.

Yes, in the case of LSP, the connection between devenv and LSP extension (via ServiceHub) has to be established first, and then devenv would invoke the ActivateAsync method via that RPC connection to activate the language server. The language server RPC can then be added as a remote target. So we have to allow for modifying the configuration after listening has started.

AArnott · 2019-08-26T20:51:06Z

src/StreamJsonRpc.Tests/JsonRpcRemoteTargetTests.cs

+    }
+
+    [Fact]
+    public async Task CanCancelOnRemoteTarget()


Testing cancellation is a good idea. I wonder if it's worth adding Progress forwarding tests once this merges with @milopezc's changes in #320.

I'll sync with @milopezc on this.

src/StreamJsonRpc/JsonRpc.cs

AArnott

A few comments as discussed

codecov-io · 2019-08-28T18:02:52Z

Codecov Report

Merging #319 into master will increase coverage by 0.34%.
The diff coverage is 94.7%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   87.38%   87.72%   +0.34%     
==========================================
  Files          33       33              
  Lines        2164     2192      +28     
  Branches      410      424      +14     
==========================================
+ Hits         1891     1923      +32     
+ Misses        217      214       -3     
+ Partials       56       55       -1

Impacted Files	Coverage Δ
src/StreamJsonRpc/Resources.Designer.cs	`66.66% <0%> (-3.83%)`	⬇️
src/StreamJsonRpc/JsonRpc.cs	`92.05% <95.72%> (+0.26%)`	⬆️
src/StreamJsonRpc/MessageHandlerBase.cs	`97.61% <0%> (+2.38%)`	⬆️
src/StreamJsonRpc/Reflection/MethodSignature.cs	`66.66% <0%> (+5.55%)`	⬆️
...treamJsonRpc/Exceptions/ConnectionLostException.cs	`75% <0%> (+25%)`	⬆️
src/StreamJsonRpc/Exceptions/RemoteRpcException.cs	`66.66% <0%> (+33.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03fc618...4ebebe1. Read the comment docs.

AArnott

This looks much better. All the issues I caught are new. :)

version.json

src/StreamJsonRpc/JsonRpc.cs

src/StreamJsonRpc.Tests/JsonRpcRemoteTargetTests.cs

src/StreamJsonRpc/JsonRpc.cs

…rget.

…eamjsonrpc into relayStream # Conflicts: # src/StreamJsonRpc/JsonRpc.cs

AArnott

Looks great!

Can you add an md file under the doc folder that explains remote targets, and find a good place in our documentation to link to it so that the file is discoverable?

Activate GitHub Actions test reporting

This reverts commit 01eeb1a, reversing changes made to 0a42cd9.

Revert "Merge pull request #319 from AArnott/betterTestingLibTemplate"

tinaschrepfer added 3 commits August 2, 2019 10:55

Merge pull request #8 from microsoft/master

362c4f5

Merge from main master

Merge pull request #9 from microsoft/master

e839861

Merge from master

Initial implementation for adding relay support to StreamJsonRpc

95e442f

tinaschrepfer requested a review from AArnott August 13, 2019 20:06

AArnott requested changes Aug 13, 2019

View reviewed changes

tinaschrepfer and others added 4 commits August 14, 2019 09:33

Added documentation, addressed some feedback.

9c91985

work in progress

28104ee

Changed the design according to review comments. Added more unit tests.

d376f5c

Merge remote-tracking branch 'upstream/master' into pr/319-relay-stream

fe7ffa9

tinaschrepfer changed the title ~~Relay stream~~ Implement ability to add remote RPC targets Aug 26, 2019

tinaschrepfer added 2 commits August 26, 2019 13:09

Fix build errors?

65c9aa0

Merge branch 'relayStream' of https://github.com/tinaschrepfer/vs-str…

5a6c96c

…eamjsonrpc into relayStream

AArnott requested changes Aug 26, 2019

View reviewed changes

Addressed feedback.

854ab0c

AArnott added 3 commits August 28, 2019 16:10

Revert unnecessary changes

e9c7e1a

Reverted change to resultDispatcherMap

27a31ec

Slight perf improvement

2c59043

AArnott requested changes Aug 28, 2019

View reviewed changes

tinaschrepfer and others added 5 commits September 3, 2019 10:58

Switched the ordering of invoking local target and invoking remote ta…

2478bd2

…rget.

Addressed feedback and added additional unit tests.

a8bf1d0

Merge branch 'relayStream' of https://github.com/tinaschrepfer/vs-str…

b4df600

…eamjsonrpc into relayStream # Conflicts: # src/StreamJsonRpc/JsonRpc.cs

Fix build errors and warnings

d420400

Merge master to resolve conflicts

30b491c

AArnott approved these changes Sep 6, 2019

View reviewed changes

Add documentation.

4ebebe1

tinaschrepfer merged commit 045c1d1 into microsoft:master Sep 6, 2019

AArnott added this to the v2.2 milestone Jun 2, 2020

AArnott added a commit that referenced this pull request Jan 14, 2025

Merge pull request #319 from AArnott/betterTestingLibTemplate

01eeb1a

Activate GitHub Actions test reporting

AArnott added a commit that referenced this pull request Jan 14, 2025

Revert "Merge pull request #319 from AArnott/betterTestingLibTemplate"

d2f4c5e

This reverts commit 01eeb1a, reversing changes made to 0a42cd9.

AArnott added a commit that referenced this pull request Jan 14, 2025

Merge pull request #321 from AArnott/betterTestingLibTemplate

b1fa3bc

Revert "Merge pull request #319 from AArnott/betterTestingLibTemplate"

AArnott mentioned this pull request Jan 14, 2025

Merge latest Library.Template #1107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ability to add remote RPC targets #319

Implement ability to add remote RPC targets #319

tinaschrepfer commented Aug 13, 2019 •

edited

Loading

AArnott left a comment

AArnott Aug 13, 2019

tinaschrepfer Aug 14, 2019

AArnott Aug 13, 2019

AArnott Aug 13, 2019

AArnott Aug 13, 2019

AArnott Aug 13, 2019

tinaschrepfer Aug 14, 2019

AArnott Aug 13, 2019

AArnott Aug 26, 2019

tinaschrepfer Aug 28, 2019

AArnott Aug 26, 2019

tinaschrepfer Aug 28, 2019

AArnott Aug 26, 2019

tinaschrepfer Aug 28, 2019

AArnott left a comment

codecov-io commented Aug 28, 2019 •

edited

Loading

AArnott left a comment

AArnott left a comment •

edited

Loading

Implement ability to add remote RPC targets #319

Implement ability to add remote RPC targets #319

Conversation

tinaschrepfer commented Aug 13, 2019 • edited Loading

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

AArnott left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 28, 2019 • edited Loading

Codecov Report

AArnott left a comment

Choose a reason for hiding this comment

AArnott left a comment • edited Loading

Choose a reason for hiding this comment

tinaschrepfer commented Aug 13, 2019 •

edited

Loading

codecov-io commented Aug 28, 2019 •

edited

Loading

AArnott left a comment •

edited

Loading