Added progress support for LSP #1

milopezc · 2019-07-10T00:15:01Z

Adding progress support on StreamJsonRpc for LSP.
Issue: microsoft/language-server-protocol#786

Support follow the following spec:
microsoft#139

src/StreamJsonRpc/IJsonRpcInstanceContainer.cs

src/StreamJsonRpc/JsonRpc.cs

src/StreamJsonRpc/MessageFormatterHelper.cs

AArnott

🕐

src/StreamJsonRpc/JsonMessageFormatter.cs

AArnott · 2019-07-10T19:51:46Z

src/StreamJsonRpc/JsonMessageFormatter.cs

+                    var reportMethod = iprogressOfTType.GetRuntimeMethod(nameof(IProgress<int>.Report), new Type[] { valueType });
+                    object typedValue = value.ToObject(valueType);
+                    reportMethod.Invoke(progress, new object[] { typedValue });
+                }


for later: this could be vastly improved as far as CPU and GC pressure cost. We can talk about options here once we're sure the design is sound. #Closed

We want to avoid all the reflection we can in this likely frequented code. The only code that should remain in this if block would be the last two lines. The rest should be computed and cached in the progressMap when it was originally stored there.

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

src/StreamJsonRpc/JsonMessageFormatter.cs

AArnott · 2019-07-12T15:22:18Z

src/StreamJsonRpc/JsonMessageFormatter.cs

@@ -429,6 +513,23 @@ private JsonRpcError ReadError(JToken json)
            };
        }

+        private void ClearProgressObject(object requestId)


object requestId [](start = 41, length = 16)

I don't see you doing any conversion within ClearProgressObject as you should to make sure that JToken, int, long or string (or whatever this number may appear as) gets converted properly. What if you just take JToken here to make it clear that the caller needn't do any conversion and then use whatever method on JToken converts strings and integers to long. #Closed

I change the param to receive a JToken object. requestId.Value<long>() will return the long value from an int or a string as long as the string represents an valid int number, otherwise it will throw a FormatException. Should I add a try-catch block for the casting? Or are we sure that we are always receiving a valid "long-castable" value there? #Resolved

That sounds great. It's OK to throw if the server sent us something we didn't expect (i.e. not an integer or string containing an integer). That will result in terminating the connection and logging out the error, which is appropriate.

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

AArnott · 2019-07-19T13:45:16Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+        /// Special method name for progress notification.
+        /// </summary>
+        private const string ProgressRequestSpecialMethod = "$/progress";
+
        /// <summary>


I see a lot of common fields between this and the JsonMessageFormatter class. Can we move these into your helper class? #Closed

AArnott · 2019-07-19T13:49:48Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+            {
+                this.rpc.NotifyAsync(ProgressRequestSpecialMethod, this.token, value).Forget();
+            }
+        }
    }


This should be shareable between the two formatters too. #Closed

AArnott · 2019-07-19T13:50:25Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+                    this.formatter.requestProgressMap.Add(this.formatter.requestIdBeingSerialized.Value, progressId);
+
+                    this.formatter.progressMap.Add(progressId, new ProgressParamInformation(value));
+


Almost all of this code should be shareable between the two formatters. #Closed

AArnott · 2019-07-19T13:52:50Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+            {
+                public readonly IMessagePackFormatter<T> Formatter;
+
+                public FormatterCache(MessagePackFormatter formatter)


Don't use the caching strategy that MessagePack documents or uses elsewhere, because your formatter is absolutely stateful and correlated to a particular JsonRpc instance. You should cache the formatters, but you should do so locally and not trying to use the .NET type system as the MessagePack library (perhaps inappropriately) does. #Closed

AArnott · 2019-07-19T13:56:16Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+                {
+                    if (MessageFormatterHelper.FindIProgressOfT(typeof(T)) != null)
+                    {
+                        // Call Get Formatter from IProgress Formatter?


// Call Get Formatter from IProgress Formatter? [](start = 24, length = 47)

Your T here may not be IProgress<T>, but at least on the client side it will be the concrete type that implements that interface. Go ahead and use a IProgressOfTFormatter<T> instance of your formatter, which must simply understand that the generic type argument is the concrete type on the client side.
You'll want to reuse that instance of the formatter every time you see that T again. I would use a Dictionary<Type, object> field (with locking) to cache these as you create them. #Closed

AArnott · 2019-08-09T18:52:09Z

src/StreamJsonRpc/MessageFormatterHelper.cs

@@ -0,0 +1,126 @@
+
+namespace StreamJsonRpc
+{


Maybe move this into the Reflection sub-namespace to remove it from the common types found in the root namespace. #Closed

AArnott · 2019-08-09T19:30:20Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+        protected const string ProgressRequestSpecialMethod = "$/progress";
+
+        /// <summary>
+        /// Object used to lock the acces to <see cref="requestProgressMap"/> and <see cref="progressMap"/>.


acces [](start = 36, length = 5)

typo #Closed

AArnott · 2019-08-09T19:32:19Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+            }
+        }
+
+        protected class ProgressParamInformation


ProgressParamInformation [](start = 24, length = 24)

Please add xml doc comments to at least all the non-private members (here and everywhere else you've added members/types) that explains things as if the consumer wasn't already familiar with them. #Closed

AArnott · 2019-08-09T19:33:30Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+            return objectType.GetTypeInfo().GetInterfaces().FirstOrDefault(i => i.IsConstructedGenericType && i.GetGenericTypeDefinition() == typeof(IProgress<>));
+        }
+
+        protected long AddProgressObjectToMap(object value)


value [](start = 53, length = 5)

If value is null, you'll eventually throw, but only after you've added something to the requestProgressMap. Can you check to ensure it's not null before doing any work? #Closed

AArnott · 2019-08-09T19:33:57Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+        protected readonly Dictionary<long, ProgressParamInformation> progressMap = new Dictionary<long, ProgressParamInformation>();
+
+        /// <summary>
+        /// Incrementable number to assing as token for the progress objects.


assing [](start = 36, length = 6)

sp: assign #Closed

AArnott · 2019-08-09T19:34:32Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+        protected readonly Dictionary<long, long> requestProgressMap = new Dictionary<long, long>();
+
+        /// <summary>
+        /// Dictionary used to map progress id token to its corresponding ProgressParamInformation instance containing the progress object and the necessary fields to report the results.


ProgressParamInformation [](start = 74, length = 24)

Use <see cref="ProgressParamInformation" /> here to help ensure the comment stays current if these types are renamed. It also helps with xml doc comments when they are converted to HTML by creating hyperlinks. #Closed

AArnott · 2019-08-09T19:35:41Z

src/StreamJsonRpc/MessageFormatterHelper.cs

+        /// <summary>
+        /// Dictionary used to map the request id to their progress id token so that the progress objects are cleaned after getting the final response.
+        /// </summary>
+        protected readonly Dictionary<long, long> requestProgressMap = new Dictionary<long, long>();


long [](start = 38, length = 4)

Can you include in the xml doc comments whether this collection only includes outbound requests? I see you're assuming here that long is the type for the message ID, which isn't true generally, but seems like a reasonable and necessary assumption for outbound requests. #Closed

AArnott · 2019-08-09T21:04:22Z

src/StreamJsonRpc/JsonMessageFormatter.cs

+    /// <remarks>
+    /// Each instance of this class may only be used with a single <see cref="JsonRpc" /> instance.
+    /// </remarks>
+    public class JsonMessageFormatter : MessageFormatterHelper, IJsonRpcAsyncMessageTextFormatter, IJsonRpcInstanceContainer


MessageFormatterHelper, [](start = 40, length = 23)

Please refactor this so that it's a helper class rather than a base class. Base classes have high impact and as formatters can only derive from one base class, a class that assists in adding functionality such as $/progress should not take the premium one spot as base class. As a helper class, it becomes an instance field on JsonMessageFormatter and thus scales better when we add other such helper classes.
Hint: we very likely will add other helpers. For example: we want to start marshaling System.IO.Stream in messages, sort of like how we're marshaling IProgress<T>. #Closed

AArnott · 2019-08-10T00:34:27Z

src/StreamJsonRpc/JsonMessageFormatter.cs

                {
-                    if (this.ProtocolVersion.Major < 2)
+                    this.requestIdBeingSerialized = Convert.ToInt64(request.Id);


Convert.ToInt64(request.Id [](start = 52, length = 26)

What happens if request.Id is null (for outbound notifications)? Will Convert.ToInt64 throw since there isn't a long that can be based on that? If it is null, we can set this.requestIdBeingSerialized to null. #Closed

Convert.ToInt64 will actually return 0 if request.Id is null. I did a quick test to confirm it.

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

We don't want it to be 0. If it is, the code won't throw an exception when Progress<T> is passed in an notification (one without an ID), and we'll end up with a memory leak. We need the field to be set to null in that case. #Closed

AArnott · 2019-08-13T23:37:46Z

src/StreamJsonRpc/Reflection/MessageFormatterHelper.cs

+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace StreamJsonRpc
+{


Let's please move this into the Reflection sub-namespace to remove it from the common types found in the root namespace. #Resolved

AArnott · 2019-08-13T23:39:33Z

src/StreamJsonRpc/Reflection/MessageFormatterHelper.cs

+    /// <summary>
+    /// Class containing useful methods to help on the implementation of message formatters.
+    /// </summary>
+    public class MessageFormatterHelper


MessageFormatterHelper [](start = 17, length = 22)

Let's rename this to MessageFormatterProgressTracker and update the xml doc comments for the type as well to indicate that this is specifically for Progress. If/when we add support for marshaling Stream, I'm pretty sure we're going to use a separate helper class to keep each class doing one thing. #Resolved

AArnott · 2019-08-13T23:56:41Z

src/StreamJsonRpc.Tests/JsonRpcTests.cs

@@ -280,6 +281,14 @@ public async Task ThrowsIfTargetNotSet()
        await Assert.ThrowsAsync<RemoteMethodNotFoundException>(() => this.serverRpc.InvokeAsync(nameof(Server.OverloadedMethod)));
    }

+    [Fact]
+    public async Task InvokeWithProgressParameter_NoMemoryLeakConfirm()
+    {


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

AArnott · 2019-08-13T23:58:10Z

src/StreamJsonRpc.Tests/JsonRpcTests.cs

+        ProgressWithCompletion<int> progress1 = new ProgressWithCompletion<int>(n =>
+        {
+            report1 = n;
+        });


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

AArnott · 2019-08-14T00:02:15Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

@@ -21,13 +25,23 @@ namespace StreamJsonRpc
    /// The README on that project site describes use cases and its performance compared to alternative
    /// .NET MessagePack implementations and this one appears to be the best by far.
    /// </remarks>
-    public class MessagePackFormatter : IJsonRpcMessageFormatter
+    public class MessagePackFormatter : MessageFormatterHelper, IJsonRpcMessageFormatter, IJsonRpcInstanceContainer


MessageFormatterHelper, [](start = 40, length = 23)

Please revise this to not use the helper class as a base class, particularly since you also have this type as a field within this class. :) #Resolved

AArnott · 2019-08-14T00:03:32Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+        /// <see cref="MessageFormatterHelper"/> instance containing useful methods to help on the implementation of message formatters.
+        /// </summary>
+        private readonly MessageFormatterHelper formatterHelper = new MessageFormatterHelper();
+


nit: readonly fields before non-readonly fields. #Resolved

AArnott · 2019-08-14T00:04:51Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+
+            return this.compress ?
+                (JsonRpcMessage)MessagePackSerializer.Typeless.Deserialize(contentBuffer.AsStream(), MessagePackSerializerOptions.LZ4Default) :
+                (JsonRpcMessage)MessagePackSerializer.Typeless.Deserialize(contentBuffer.AsStream());
        }


For each time we look at compress and do something different, let's replace that with just a MessagePackSerializerOptions field set based on the compress parameter passed in to our constructor. Then all the branching that appears elsewhere in the class can go away. #Resolved

AArnott · 2019-08-14T00:08:01Z

src/StreamJsonRpc.Tests/MessagePackFormatter.cs

+        private class StandardPlusIProgressOfTResolver : IFormatterResolver
+        {
+            private readonly MessagePackFormatter formatter;
+            private Dictionary<Type, object> progressFormatterCache = new Dictionary<Type, object>();


private [](start = 12, length = 7)

add readonly modifier here. #Resolved

AArnott

milopezc · 2019-08-15T06:51:04Z

Closing since there is a new PR targeting the microsoft/master branch:
microsoft#320

Added progress support for LSP

ffaff4f