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.

Sign up for GitHub

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

Added progress support for LSP #1

Closed
wants to merge 13 commits into from
Closed

Added progress support for LSP #1

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ffaff4f
Added progress support for LSP
milopezc Jul 10, 2019
4f2eb72
Several code fixes on progress support
milopezc Jul 10, 2019
aa1f904
Few code fixes for IProgress support
milopezc Jul 12, 2019
cd09aa2
Several implementation and test fixes
milopezc Jul 16, 2019
2c1476b
More code and test fixes for Progress support
milopezc Jul 17, 2019
434bfb3
Fixes on progress support implementation
milopezc Jul 17, 2019
99fea2c
Merge branch 'master' into ProgressSuportOnLSP
milopezc Jul 18, 2019
fb40a13
Added Progress support on MessagePack
milopezc Jul 19, 2019
8ec6b86
Fixes for streaming support on MessagePack
milopezc Jul 19, 2019
9b96a9d
Resolved merge from upstream master
milopezc Aug 6, 2019
b4ba6cf
Several code fixes
milopezc Aug 9, 2019
1cc5b4d
Fixed assignment to requestIdBeingSerialized
milopezc Aug 12, 2019
8504512
Code and style fixes
milopezc Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcClient20InteropTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.Threading;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using StreamJsonRpc;
Expand Down Expand Up @@ -149,6 +151,93 @@ public async Task InvokeWithParameterPassedAsObjectAsync_NoParameter()
Assert.Null(request["params"]);
}

[Fact]
public async Task InvokeWithProgressParameterAsArray()
{
AsyncAutoResetEvent signal = new AsyncAutoResetEvent();

int sum = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report =>
{
sum += report;
signal.Set();
});

int n = 3;
Task<int> invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { n, progress });

JToken request = await this.ReceiveAsync();

JToken progressID = request["params"][1];

// Send responses as $/progress
int sum2 = 0;
for (int i = 1; i <= n; i++)
{
string content = "{ \"jsonrpc\": \"2.0\", \"method\": \"$/progress\", \"params\": { \"token\": " + progressID + ", \"value\": " + i + " } }";
JObject json = JObject.Parse(content);

this.Send(json);

sum2 += i;

await signal.WaitAsync().WithCancellation(this.TimeoutToken);
Assert.Equal(sum2, sum);
}

this.Send(new
{
jsonrpc = "2.0",
id = request["id"],
result = sum,
});

int result = await invokeTask;
Assert.Equal(sum2, result);
}

[Fact]
public async Task InvokeWithProgressParameter()
Copy link

@AArnott AArnott Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

InvokeWithProgressParameter [](start = 22, length = 27)

all the same comments as the test method above.
There's enough in common between these tests that you may want to refactor them to share a helper method that just takes a delegate that does the original client invocation but does the rest exactly the same. #Closed

{
int sum = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report =>
{
sum += report;
});

int n = 3;
Task<int> invokeTask = this.clientRpc.InvokeWithParameterObjectAsync<int>("test", new { Bar = n, Progress = progress });

JToken request = await this.ReceiveAsync();

long progressID = request["params"]["Progress"].Value<long>();

// Send responses as $/progress
int sum2 = 0;
for (int i = 0; i < n; i++)
{
string content = "{ \"jsonrpc\": \"2.0\", \"method\": \"$/progress\", \"params\": { \"token\": " + progressID + ", \"value\": " + i + "} }";
JObject json = JObject.Parse(content);

this.Send(json);

sum2 += i;
}

System.Threading.Thread.Sleep(1000);
Assert.Equal(sum2, sum);

this.Send(new
{
jsonrpc = "2.0",
id = request["id"].Value<long>(),
result = sum,
});

int result = await invokeTask;
Assert.Equal(sum2, result);
}

[Fact]
public void NotifyWithParameterPassedAsObjectAsync_ThrowsExceptions()
{
Expand Down
173 changes: 173 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Serialization;
using System.Text;
using System.Threading;
Expand Down Expand Up @@ -280,6 +281,14 @@ public async Task ThrowsIfTargetNotSet()
await Assert.ThrowsAsync(typeof(RemoteMethodNotFoundException), () => this.serverRpc.InvokeAsync(nameof(Server.OverloadedMethod)));
}

[Fact]
public async Task InvokeWithProgressParameter_NoMemoryLeakConfirm()
{
Copy link

@AArnott AArnott Aug 13, 2019 •
edited by milopezc
Loading

Choose a reason for hiding this comment

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

Please add a NotifyAsyncWithProgressParameter_NoMemoryLeakConfirm test as well.
hint: it will throw an exception since callers aren't allowed to pass Progress objects in Notify methods, but we want to confirm that there is no leak after that exception is thrown. #Resolved

WeakReference weakRef = await this.InvokeWithProgressParameter_NoMemoryLeakConfirm_Helper();
GC.Collect();
Assert.False(weakRef.IsAlive);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
Expand Down Expand Up @@ -834,6 +843,104 @@ public async Task InvokeWithParameterObject_DefaultParameters()
Assert.Equal(12, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressParameter()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report = n;
});

int result = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressParameter), new { p = progress }, this.TimeoutToken);

Assert.Equal(1, report);
Assert.Equal(1, result);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressParameterMultipleRequests()
{
int report1 = 0;
ProgressWithCompletion<int> progress1 = new ProgressWithCompletion<int>(n =>
{
report1 = n;
});
Copy link

@AArnott AArnott Aug 13, 2019 •
edited by milopezc
Loading

Choose a reason for hiding this comment

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

tip: you can shrink this syntax from 4 lines to 1: (n => report1 = n) #Resolved


int report2 = 0;
ProgressWithCompletion<int> progress2 = new ProgressWithCompletion<int>(n =>
{
report2 = n * 2;
});

int report3 = 0;
ProgressWithCompletion<int> progress3 = new ProgressWithCompletion<int>(n =>
{
report3 = n * 3;
});

await this.InvokeMethodWithProgressParameter(progress1);
await this.InvokeMethodWithProgressParameter(progress2);
await this.InvokeMethodWithProgressParameter(progress3);

await progress1.WaitAsync();
await progress2.WaitAsync();
await progress3.WaitAsync();

Assert.Equal(1, report1);
Assert.Equal(2, report2);
Assert.Equal(3, report3);
}

[Fact]
public async Task InvokeWithParameterObject_InvalidParamMethod()
Copy link

@AArnott AArnott Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

InvokeWithParameterObject_InvalidParamMethod [](start = 22, length = 44)

I think the exception thrown is reasonable. Can you check the trace output of the test and share what it prints? I'm hoping on the server-side it gives a clear idea of why the method expected to take this wasn't selected. #Closed

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure, this is the output of the test:
Server Information: 3 : Listening started.
Client Information: 3 : Listening started.
Client Information: 8 : {"id":2,"method":"MethodWithInvalidProgressParameter"}
Client Verbose: 8 : Sent: {
"jsonrpc": "2.0",
"method": "MethodWithInvalidProgressParameter",
"params": {
"p": {}
},
"id": 2
}
Server Information: 7 : {"id":2,"method":"MethodWithInvalidProgressParameter"}
Server Verbose: 7 : Received: {
"jsonrpc": "2.0",
"method": "MethodWithInvalidProgressParameter",
"params": {
"p": 0
},
"id": 2
}
Server Information: 6 : Received request "2" for method "MethodWithInvalidProgressParameter".
Server Warning: 5 : Invocation of "MethodWithInvalidProgressParameter" cannot occur because arguments do not match any registered target methods.
Server Information: 8 : {"id":2,"error":{"code":-32602,"message":"Unable to find method 'MethodWithInvalidProgressParameter/1' on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
Server Verbose: 8 : Sent: {
"jsonrpc": "2.0",
"error": {
"code": -32602,
"message": "Unable to find method 'MethodWithInvalidProgressParameter/1' on {no object} for the following reasons: An argument was not supplied for a required parameter.",
"data": null
},
"id": 2
}
Client Information: 7 : {"id":2,"error":{"code":-32602,"message":"Unable to find method 'MethodWithInvalidProgressParameter/1' on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
Client Verbose: 7 : Received: {
"jsonrpc": "2.0",
"error": {
"code": -32602,
"message": "Unable to find method 'MethodWithInvalidProgressParameter/1' on {no object} for the following reasons: An argument was not supplied for a required parameter.",
"data": null
},
"id": 2
}
Client Warning: 12 : Received error response for request 2: InvalidParams "Unable to find method 'MethodWithInvalidProgressParameter/1' on {no object} for the following reasons: An argument was not supplied for a required parameter.":
Server Information: 13 : Connection closing (LocallyDisposed: Stream has been disposed).
Client Information: 13 : Connection closing (LocallyDisposed: Stream has been disposed).

In reply to: 304388083 [](ancestors = 304388083)

Copy link

@AArnott AArnott Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm... that error is not super-great, given the client did supply a value. We might want to find the code that's emitting that message and see if we can fine-tune it to distinguish between "no value" and "incompatible value". #Closed

Copy link
Owner Author

@milopezc milopezc Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I found this in the JsonRpc class:
return new JsonRpcError { Id = request.Id, Error = new JsonRpcError.ErrorDetail { Code = JsonRpcErrorCode.InvalidParams, Message = targetMethod.LookupErrorMessage, }, };

The targetMethod.LookupErrorMessage contains the message that is being thrown in the Exception, maybe we could customize that message just like we do for the MethodNotfoundError:

return new JsonRpcError { Id = request.Id, Error = new JsonRpcError.ErrorDetail { Code = JsonRpcErrorCode.MethodNotFound, Message = string.Format(CultureInfo.CurrentCulture, Resources.RpcMethodNameNotFound, request.Method), }, };
#Resolved

Copy link

Choose a reason for hiding this comment

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

Sure. If you make any change in this area please review the messages in the relevant cases to make sure it all makes sense. IMO this is optional, but a nice to have.

In reply to: 304577320 [](ancestors = 304577320)

{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report = n;
});

await Assert.ThrowsAsync<RemoteMethodNotFoundException>(() => this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithInvalidProgressParameter), new { p = progress }, this.TimeoutToken));
}
Copy link

@AArnott AArnott Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The preferred syntax for such an assertion is:

await Assert.ThrowsAsync<RemoteMethodNotFoundException>(() => this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithInvalidProgressParameter), new { p = progress }, this.TimeoutToken));
``` #Closed


[Fact]
public async Task InvokeWithParameterObject_ProgressParameterAndFields()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report += n;
});

int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new { p = progress, x = 2, y = 5 }, this.TimeoutToken);

Assert.Equal(7, report);
Assert.Equal(7, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressAndDefaultParameters()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report += n;
});

int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new { p = progress, x = 2 }, this.TimeoutToken);

Assert.Equal(12, report);
Assert.Equal(12, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ClassIncludingProgressProperty()
{
int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new XAndYFieldsWithProgress { x = 2, y = 5, p = new Progress<int>() }, this.TimeoutToken);
Assert.Equal(7, sum);
}

[Fact]
public async Task CanInvokeServerMethodWithNoParameterPassedAsArray()
{
Expand Down Expand Up @@ -1402,6 +1509,26 @@ private void StartListening()
this.clientRpc.StartListening();
}

[MethodImpl(MethodImplOptions.NoInlining)]
private async Task<WeakReference> InvokeWithProgressParameter_NoMemoryLeakConfirm_Helper()
{
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report => { });

WeakReference weakRef = new WeakReference(progress);

int invokeTask = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressParameter), new { p = progress });

// Clear progress variable locally
progress = null;

return weakRef;
}

private async Task InvokeMethodWithProgressParameter(IProgress<int> progress)
{
await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressParameter), new { p = progress }, this.TimeoutToken);
}

public class BaseClass
{
protected readonly TaskCompletionSource<string> notificationTcs = new TaskCompletionSource<string>();
Expand Down Expand Up @@ -1459,6 +1586,38 @@ public static int MethodWithDefaultParameter(int x, int y = 10)
return x + y;
}

public static int MethodWithProgressParameter(IProgress<int> p)
Copy link

@AArnott AArnott Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

MethodWithProgressParameter(IProgress p) [](start = 26, length = 45)

We should create a new nested class (sibling to the Server class) that defines just one method which takes a Progress<int> parameter, and then add a test that verifies that this fails when added as a target object in some acceptable way, since we only support server methods that accept IProgress<T> and not the concrete type Progress<T>. #Closed

{
p.Report(1);
return 1;
}

public static int MethodWithProgressAndMoreParameters(IProgress<int> p, int x, int y = 10)
{
int sum = x + y;
p.Report(x);
p.Report(y);
return sum;
}

public static int MethodWithProgressArrayParameter(params IProgress<int>[] progressArray)
{
int report = 0;

foreach (IProgress<int> progress in progressArray)
{
report++;
progress.Report(report);
}

return 0;
}

public static int MethodWithInvalidProgressParameter(Progress<int> p)
{
return 1;
}

public int? MethodReturnsNullableInt(int a) => a > 0 ? (int?)a : null;

public int MethodAcceptsNullableArgs(int? a, int? b) => (a.HasValue ? 1 : 0) + (b.HasValue ? 1 : 0);
Expand Down Expand Up @@ -1786,6 +1945,20 @@ public class XAndYFields
#pragma warning restore SA1307 // Accessible fields should begin with upper-case letter
}

[DataContract]
public class XAndYFieldsWithProgress
{
// We disable SA1307 because we must use lowercase members as required to match the parameter names.
#pragma warning disable SA1307 // Accessible fields should begin with upper-case letter
[DataMember]
public int x;
[DataMember]
public int y;
[DataMember]
public IProgress<int> p;
#pragma warning restore SA1307 // Accessible fields should begin with upper-case letter
}

internal class InternalClass
{
}
Expand Down
Loading