Spec proposal: support for IProgress<T> about vs-streamjsonrpc HOT 42 CLOSED

I agree: it doesn't require an extension. My proposal is to spec out the protocol so that multiple libraries can interoperate using a standard pattern.

from vs-streamjsonrpc.

As for your beefed up 3-member interface, can you explain what each member would mean? We can't collect from the client or provider this on to the server side if the API contract is IProgress<T> as we want it to be on .NET. So I'd prefer the interface be minimal, indicating just that the client is interested in progress notifications and the means by which to associate them with the request.

If the server would cater to switches from the client to customize progress reporting, I think that can and should be done as separate arguments, allowing an API in .NET or TypeScript, both on the client and server, to look like this:

interface ISomeService
{
   SomeServerOperationAsync(
      arg1: string,
      arg2: int,
      progress: Progress<StepProgress>?,
      progressSwitch1: bool?,
      progressSwitch2: bool?);
}

This way whether .NET's IProgress<T>, VSCode's own Progress<T> type, or any other platform's native mechanism for reporting progress, the client can instantiate one and pass it in, the native JSON-RPC library can translate that into a { "__jsonrpc__progress": <token> } object, and the server-side can translate it back into a native object useful for reporting progress and the extra args can tell the server how the client would like to customize it, if necessary.

What I also like about this protocol is that you don't actually need native library support. For example if the server-side uses a JSON-RPC library that doesn't support this progress protocol extension, the server method can simply accept the { "__jsonrpc__progress": <token> } directly as an argument, and now has everything it needs to send progress notifications back to the client. This is why I agree with your earlier idea that the progress token not be forcibly linked to be the request ID, since the server method likely has no visibility into that request ID value.
And once we've decoupled the request ID from the progress token (at a protocol level at least), we open up the possibility of multiple progress arguments in case some server offers different kinds, frequencies, etc. of progress based on which progress argument(s) are passed in. I compare this to multiple loggers in msbuild: you can have msbuild log to the console, and three different files, all at different verbosity levels. Over RPC logging the same event over multiple progress args may not be the most efficient way to do it, and more efficient ways by setting a max verbosity level in a client-provided argument and an actual verbosity level in each progress update is probably advisable, it's nevertheless a possibility that folks might find a good fit in their application.

from vs-streamjsonrpc.

we would be closing the door on progress on notifications.

Very valid point.

Then I opt to stick with not having the property at all. I agree that the client can do the correlation as well.

from vs-streamjsonrpc.

@AArnott I started to spec / implement this for LSP and have a question regarding the $/progress notification.

{
  "jsonrpc": "2.0",
  "method": "$/progress",
  "params": {
    "progressToken": "some-JSON-token",
    "value": { "some": "status-token" }
  }
}

Should we name progressToken as token? It is already scope by the $/progress method.

from vs-streamjsonrpc.

@dbaeumer is this something you already have a spec for? Any feedback?

from vs-streamjsonrpc.

I've left a message on the json-rpc spec forum.

from vs-streamjsonrpc.

Hi. :)
Initial feedback... this seems like just an app/endpoint specific implementation detail, not sure it requires a larger specification extension... but probably warrants documenting the pattern/method signature and intent if you want to see it implemented more broadly.

from vs-streamjsonrpc.

@AArnott yes, but the discussion is currently LSP specific. Major reason is that progress needs to be reported for things that are not necessarily bound to a request. Consider that a server builds an index on startup and wants to report corresponding progress.

The discussion happens here: microsoft/language-server-protocol#70

from vs-streamjsonrpc.

@dbaeumer This isn't directly related to the LSP conversation you referenced. I'm proposing a generalized progress reporting system. I am puzzled though that for LSP you say it may not be bound to a request, since such a condition is directly stated in the title of your issue.

from vs-streamjsonrpc.

In your example, what would WorkUpdate-token be?

from vs-streamjsonrpc.

@felixfbecker: That's an application protocol level detail. It's whatever the server wants to return to the client to communicate progress. It might be a number indicating % progress completed. It could be a JSON object with the additional data that is ready for processing.

from vs-streamjsonrpc.

Found my way here from the JSON-RPC forum, figured I'd offer up the anecdotal experience that I've seen basically this implemented before and like @mpcm said: it can be done within the existing spec without extension.

from vs-streamjsonrpc.

Seems to me an application level pattern doesn't need to be formalized in the underlying protocol, or even be implemented in the direct JSON-RPC libraries. That's not to say that I don't think that a more formal pattern would be a bad thing. In fact I think it would be good to see a protocol built on top of the JSON-RPC protocol, and likewise libraries for that protocol built on top of existing JSON-RPC libraries (which should be not too difficult, as that's what many people do today).

from vs-streamjsonrpc.

Consider this existing implementation from Live Share.

from vs-streamjsonrpc.

Thanks for the pointer.

from vs-streamjsonrpc.

The current version of the next libraries contain a first cut of progress reporting support. The corresponding implementation and spec part can be found here:

https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.progress.proposed.md#L1

https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.progress.proposed.ts#L1

https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/progress.proposed.ts#L1

This currently allows to initiate progress from the server to the client. To bind progress to a request the idea is to add a progressId / progressToken property to the request params literal that a server then can use to report specific progress for a request.

Let me know if this addresses your problems.

from vs-streamjsonrpc.

It looks like more discussion is happening over at https://github.com/microsoft/ms-lsp/issues/2

from vs-streamjsonrpc.

Thanks for the link, @dbaeumer. It looks like your work there is pretty unrelated to this proposal after all, if I'm reading it correctly. Your proposal is based on the server driving the client's global UI in terms of some "we're busy" progress bar, without any context to a particular RPC request that preceded it.
For my proposal, the direction is the opposite: an RPC client is asking the server to execute a method and provide progress updates regarding that specific call with no contract (expressed or implied) about any UI that might be showing on the client. The server can provide progress updates that include partial results and/or % completion. The client knows which of these to expect and can aggregate partial results and/or consider displaying % completion if they want in the UI that that particular command is associated with on the client app (if any).

from vs-streamjsonrpc.

I've refined my spec proposal in the issue description and plan to start implementation soon.

from vs-streamjsonrpc.

@AArnott actually providing progress for a specific call will be available as well (not implemented yet). The difference is that the proposal adds that to LSP itself and not to JSON-RPC. How it will work is that if the client sends a request and the client will provide UI to report progress for that request the request params will contain an additional property progressId or progressToken. I think that this is very similar to what you outlined in the description of this issue. The difference are:

• a separate id / token instead of reusing the JSON-RPC message id
• the property will only be available if the client actually shows progress for the request

I also looked into whether reporting progress and reporting partial results should be combined and I decided against it. Major reason being is that I think that there is not always a direct correlation between the progress of an operation and its final result set.

from vs-streamjsonrpc.

@dbaeumer Can you share a link to the code or spec or discussion you're talking about for the progressId/progressToken feature? I can't find anything about that in any of the links you've shared so far.

I agree what you just described sounds much closer to what I'm attempting here. And I'd love to come up with compatible solutions.

I also looked into whether reporting progress and reporting partial results should be combined and I decided against it. Major reason being is that I think that there is not always a direct correlation between the progress of an operation and its final result set.

The spec here doesn't define one way or the other. I'm leaving it up to the server to define what results and progress are actually returned to the client. If I understand you correctly, an example of the no direct correlation between the two could be that progress is merely an integer between 0 and 100 while the final result set is 100,000 lines of text. Or perhaps the progress is each fragment found while the final result set is sorted. The latter would be highly redundant and perhaps not the best use of traffic, but that's why I'm leaving it up to the server to decide and document how the two relate to each other. Would that mitigate your concern?

the property will only be available if the client actually shows progress for the request

It sounds like you're saying where my spec is that the client specifies null as the parameter value, you just omit the parameter altogether. That works only for LSP because you always used named arguments, whereas in the general case ordered arguments (i.e. a JSON array) is often used in the request. Given that, we need a reserved slot if the "please send progress" part of the request is in the params token of the request at all.
Further, while in your javascript/TypeScript server implementation is presumably taking an object with all these named arguments and can thus fish out the progressId property if there is one and start sending notifications with that ID, in general JSON-RPC servers often prefer a list of explicit parameters in their method signature instead of a JSON object. For languages that support method overloading, this allows multiple overloads, more declarative intent and docs on what arguments are expected, and allows the JSON-RPC library on the server to automatically reject requests whose params don't conform to a supported schema. So if we are to supported ordered arguments at the server side, we once again need a designated ordering for reporting this value that indicates that the client wants progress.

a separate id / token instead of reusing the JSON-RPC message id

I can see how you came to that, given your design is to be completely application level and thus for the server method to be able to send notifications, it needs to know the progress token, and it has no visibility into the request ID of the original message.

In .NET, it's very natural to reuse IProgress<T> functionality here, and a JSON-RPC library can see that the server method signature takes one of these parameters and can use that to properly interpret the request argument (of {} or null) and synthesize an IProgress<T> object to pass to the server method. This is great for the server method, because now they can simply call progress.Report(myProgress) without regard to how that works or even that JSON-RPC is behind it. Since JSON-RPC is behind it, it has visibility into the request ID and thus no need for a special progress token is necessary.

But now that I'm talking to you, I realize this may not be compatible with a TypeScript app/library, since at runtime there are no types to parameters in the method signature. And even if we could somehow divine that from a method signature, we surely couldn't if the RPC server method just took an arbitrary JSON object as one parameter as LSP servers do. So your spec works well for TypeScript&LSP but is incompatible with ordered args at the JSON-RPC layer, and isn't the greatest experience in other languages. Mine doesn't work for TypeScript at all. How to reconcile these and get one spec that works everywhere?

I guess I'll finish here rather than brainstorming at the moment to give you a chance to confirm/refute any of my points above.

from vs-streamjsonrpc.

How about this modification to my spec above as a solution to add support for TypeScript:

-The progress argument is {} or null to indicate that the client supports and wants progress notifications or does not, respectively.
+The progress argument is {progressToken: <token>} or null to indicate that the client supports and wants progress notifications or does not, respectively.

-id must match the value of the request that progress is being reported for.
+id must match the value of the progressToken supplied in the request that progress is being reported for.

This allows TypeScript based apps and libraries to keep working since they'll just pass this {progressToken: <token>} object around as a regular argument and the server method can crack it open and call back into the JSON-RPC library to send notifications with it. It'll work both client and server side.
Note we keep the positional argument support because we're not defining a name for the argument in the spec, we're only defining the schema for the argument's value. The server still defines what the argument name and/or position will be.
In .NET or any other strongly-typed language a JSON-RPC language could recognize the IProgress<T> type in the method signature and automatically "serialize" to this pattern. It could even simply reuse the requestId as the progressToken to simplify its own bookkeeping.

Thoughts?

from vs-streamjsonrpc.

OK, after talking with @jasongin, I've modified the spec in the issue description. I think this should allow even more functionality and work for all languages/libraries. It would even work with a JSON-RPC library that doesn't have progress reporting support built-in, but where it is built-in, the library can make the RPC server and calling client very simple by exposing native Progress functionality.

from vs-streamjsonrpc.

There has not been a lot of discussions around this. It happened in this PR microsoft/vscode-languageserver-node#261 and in a comment here: microsoft/vscode-languageserver-node#261 (comment)

I was looking at progress from two viewpoints:

1. partial results for a request. E.g. a request returns many elements and it want to report a subset as soon as it is available. The data usually conforms to the response result.
2. providing UI progress for a progress bar. The data depends how progress is reported.

For the first one I would definitely use a $/progress notifications where the id in the notification is the id of the request.

For the second one I am not so sure due to the following reasons:

• progress might not be bound to a request / response. For example a server might want to report progress on a background indexing although no active request is pending.
• folding both would require that libraries dispatch on the structure of the value property of the $/progress notification instead of dispatching on the method value. In general I tried to avoid leaking $/... methods to the outside of the JSON-RPC library. For cancellation the TS implementation of the library uses cancellation tokens.

So far my goal was to keep server initiate UI progress (work done) the same as reporting UI progress for a request. This is why I was thinking about a progressToken in the request params. But may be things are easier if we keep progress (independent of kind) for request together and have server initiated UI progress different. Then the $/progress notification always refers with the id to a request.

Need to think a little :-)

from vs-streamjsonrpc.

Looking at folding UI progress and partial result progress raises the problem that one tagging interface

interface IJsonRpcProgress
{
    __jsonrpc__progressToken: any
}

makes it hard to convey which of those the client accepts. For example in LSP we might have messages that accept UI progress but no partial results. We could easily drop UI progress on the client side, but accepting partial results where the editor has no corresponding API might complicate things. We could encapsulate this into a library but this needs then to be done for every implementation language. Alternatively we could have

interface JsonRpcProgress {
    __jsonrpc__workCompleted?: boolean;
    __jsonrpc__partialResults?: boolean;
    __jsonrpc__generic?: boolean;
}

which makes it more clear for the server what is supported.

from vs-streamjsonrpc.

I don't think we should combine these two forms of progress (reporting on a request, and pushing progress to client UI based on some server-initiated operation). The latter seems to clearly be app-specific and trivially implemented by sending a notification from server to client for a method and params of the client's choosing (e.g. setGlobalProgress). As each client may present progress differently and have its own servers that want to represent their progress in different ways, leaving this strictly at the app level seems like a good fit. LSP as a protocol that builds on top of JSON-RPC could certainly define what this should look like.

So I'm primarily focused on the request-progress-response use case. More on that in my next message.

from vs-streamjsonrpc.

The idea of these where as follows:

• __jsonrpc__workCompleted: client accepts work done progress which is usually presented in the UI.
• __jsonrpc__partialResults: client accepts partial result progress (e.g. the result is an array and the result can be delivered in batches).
• __jsonrpc__generic: the client accept Progress in a generic way specific to the request.

I proposed this since implementation wise I would like to encapsulate progress in the library and not expose these directly to the user of the lib. I do the same with cancellation in the vscode-jsonrpc library. The fact that there is a $/cancel message is not visible to the outside and I would like to keep it the same for $/progress. This way the id of a JSON-RPC message can be used.

This would lead in TS to a handler signature like this:

interface RequestHandler {
   (params: P, cancellationToken: CancellationToken, workDoneToken: WorkDoneToken, partialResultToken?: PartialResultToken)
}

If partialResultToken === undefined the request doesn't support partial result reporting.

If I understand your proposal correctly I can only have a signature:

interface RequestHandler {
   (params: P, cancellationToken: CancellationToken, progressToken: ProgressToken);
}

with some values in the params (may be even application specific) indicating what kind of progress can be reported.

I personally would like to at least make partial result reporting a first class citizen in the vscode-jsonrpc library as cancellation is. Work done progress might be different since how work done is reported differs from application model to application model. This is why I had work done progress as a separate token in the params.

This is independent of server initiated progress which I agree can't be map onto this and is best implemented using special messages.

from vs-streamjsonrpc.

I proposed this since implementation wise I would like to encapsulate progress in the library and not expose these directly to the user of the lib.

The fact that there is a $/cancel message is not visible to the outside and I would like to keep it the same for $/progress.

We share that goal. StreamJsonRpc hides the $/cancel method as well.

This way the id of a JSON-RPC message can be used.

You lose two things with this statement: the ability to support multiple progress arguments in a single method call, and (perhaps more importantly), the freedom for an application to interop properly via this protocol even if the library doesn't support it.
How would you compare the tradeoff of this design decision?

If partialResultToken === undefined the request doesn't support partial result reporting.

What is this token? How does the server method use it? Do they have to call back into the JSON-RPC library and pass this in as an argument along with whatever progress they have to report?
You mentioned you had a goal of implementing it all in the library. I'm going beyond that: I'd like even the library itself to be something the server can be ignorant of. So instead of passing in some sort of a token, you pass in a Progress<T> (which I believe vscode already has) so that the server method can just call partialResultProgress.report(someValue). This can be done whether or not the caller of the server method is a json-rpc library or some other component within the same process without using RPC.

If I understand your proposal correctly I can only have a signature:
interface RequestHandler {
(params: P, cancellationToken: CancellationToken, progressToken: ProgressToken);
}

I don't see my proposal as limited to that one. In fact I don't think that is even one of the valid ones.

JSON-RPC patterns aside

I'm starting from the JSON-RPC spec which suggests that methods are invoked with either named or positional arguments. See section 4.2 of the spec. So a server method would always have one parameter for each argument that the client is passing in:

myServerMethod(arg1: string, arg2: int);

Then per the JSON-RPC spec, the request can provide positional arguments or named arguments, and the responsibility of the JSON-RPC library is to line those up with the method on this signature. Now, Javascript makes positional arguments easy, but AFAIK named arguments are impossible to implement this way in javascript because there is no reflection that will tell you the parameter names. In .NET we can easily support both.

It seems vscode-jsonrpc went a (creatively) different direction by instead of passing in the request messages parameters as regular parameters to the method, it always passes in a single parameters object. This gives you named arguments support in javascript, but (I think?) at the expense of being unable to support positional arguments (unless P in your params: P is allowed to be an array I guess). Does vscode-jsonrpc support positional arguments?

Back to the discussion at hand

Anyway in StreamJsonRpc I expose CancellationToken to the server method as just one more parameter among all its others. The behavior of this CancellationToken is "entirely encapsulated in the library", but accessing it is very natural. I was expecting to expose this progress parameter the same way. So for example, a C# server method would look like this:

Task MyServerMethodAsync(string arg1, int arg2, IProgress<MyDataProgress> progress = null, CancellationToken cancellationToken = null)

I expect that a typical json-rpc library for TypeScript/Javascript would express it in nearly an identical way. But whereas in .NET we would recognize the need to specially interpret the progress argument by virtue of the type of the parameter on the server method, in Javascript of course that's not an option, and you could instead recognize the special schema conformed to by the progress argument as it came in, and instantiate a Progress<T> reporting object and pass that in as the argument.

For vscode-jsonrpc which uses different signatures, I nevertheless expected progress to still be just one of the parameters like the others, leaving the interface of the server method like this:

interface RequestHandler {
   (params: P);
}

interface P {
   arg1: string,
   arg2: int,
   progress: Progress<MyData>
}

See, I don't think progress, or cancellation for that matter, should be kept separate from the rest of the parameters on the server method. Both of these concepts aren't unique to JSON-RPC. Any method may want to take such arguments. So why not just let it define its parameters, including progress (and cancellation, but I'm not trying to change the past) and let JSON-RPC be one of the means for calling that method, without that method having to structure its signature around the specific library that it expects to be called with?

In your proposal, @dbaeumer, what does the client code that calls this method look like, including setting up and receiving progress? My goal is to make it look as absolutely natural as can be. And I'm hoping that TypeScript can achieve it with the spec we end up with here. In .NET, it's totally natural:

// doesn't care about cancellation or progress
await server.MyServerMethodAsync("arg1", arg2: 51);

// cares
var progress = new Progress<MyDataProgress>(data => { /* we got progress! */ });
await server.MyServerMethodAsync("arg1", arg2: 51, progress, cancellationToken);

Note that there is absolutely no RPC awareness at all. Or if you don't have a client proxy, then it would look like this:

var progress = new Progress<MyDataProgress>(data => { /* we got progress! */ });
await jsonRpc.InvokeWithCancellationAsync("MyServerMethodAsync", "arg1", arg2: 51, progress, cancellationToken);

If the server supports reporting "work done" progress as well as "partial result" progress, it can just take another IProgress<T2> parameter, which is what it would have done all by itself, without any thought for an RPC library.

Task MyServerMethodAsync(string arg1, int arg2, IProgress<MyDataProgress> progress = null, IProgress<WorkProgress> workProgress = null, CancellationToken cancellationToken = null)

Or if the server method were willing to report one kind of progress, but at varying verbosity levels, the server would of course take an extra parameter for that:

Task MyServerMethodAsync(string arg1, int arg2, IProgress<MyDataProgress> progress = null, ProgressVerbosityLevel verbosity = 1, CancellationToken cancellationToken = null)

So I'm curious whether you share the goal and feel you can meet it of having syntax that is as natural on its platform with either of our proposals.

from vs-streamjsonrpc.

Does vscode-jsonrpc support positional arguments?

Yes, vscode-jsonrpc fully support positional parameters. The TS signatures that are type checked are limited to 10 though (see https://github.com/Microsoft/vscode-languageserver-node/blob/master/jsonrpc/src/main.ts#L291).

The reason why I went with one param object was the flexibility of additional parameters without dealing with their position. It is basically a work around for named parameters.

I do agree that a generic Progress gives you all the flexibility. The problem I have is that at least in a JS/TS implementation the dispatching has to happen by inspecting properties in JSON literals or by remembering tokens and not by method name. Currently different types of things are dispatch by different method names and for me reporting partial progress was always different than reporting work done.

I also believe that having one generic interface / mechanism is harder to understand and needs more documentation (at least in LSP since all I can spec is data that flows over the wire).

To support both progress styles in LSP using one generic $/progress method I need to spec the additional params using the proposed __jsonrpc__progressToken tagging style. That might look like this:

{
    workDone?: {
		__jsonrpc__progressToken: string | number;
	},
    partialResult?: {
		__jsonrpc__progressToken: string | number;
    }
}

in contrast to two separate methods $/progress and $/workDone and booleans to indicate what is supported.

The same needs to be done even if I would allow to use positional parameters for result progress and work done progress. The spec would still need to say that the second parameter is for workDone progress having the tagging interface and the third is for partialResult progress having the tagging interface.

I hope that clarifies my thinking around it. I am not trying to say this will not work for LSP and can't be implemented. It is IMO harder to explain, to document and needs more work in the json-rpc library.

from vs-streamjsonrpc.

The problem I have is that at least in a JS/TS implementation the dispatching has to happen by inspecting properties in JSON literals or by remembering tokens and not by method name. Currently different types of things are dispatch by different method names and for me reporting partial progress was always different than reporting work done.

It makes sense since Javascript doesn't have method overloads that you want to give different names to methods that take different parameter types. In your preferred spec, I believe you last expressed it as:

interface JsonRpcProgress {
    __jsonrpc__workCompleted?: boolean;
    __jsonrpc__partialResults?: boolean;
    __jsonrpc__generic?: boolean;
}

Is that correct? Further (and please correct me if I'm wrong):

1. The above object would appear as the value for a specific, fixed-name parameter alongside the others in JSON-RPC, but at least for vscode-jsonrpc users the parameter would be separate from the others, and alongside the CancellationToken
2. A server that supports this would know the 2-3 hard-coded method names that the client is expecting progress to be sent back on.
3. The server would have access to the id of the request message in order to invoke one of those client progress callback methods while passing that id back to it.
4. The client would have to have progress support built into the library in order to match the callback from the server with the original request by id.

IMO the above constraints seem both burdensome and limiting. And it seems obviously tailored for the LSP case where you have a design that strongly favors two distinct types of progress that might be reported.

But I think you can do it in the more general way, and naturally even in TypeScript. Let me sketch it out in TS/JS and you can tell me if it helps or if I went off the rails somewhere. In particular as your concern seems to focus on one method that could take two different types of progress data, I'll demonstrate how that needn't be the case. I'm going to make-up the syntax for using the Progress<T> that I think you have available in TypeScript. I'm not too familiar with vscode-jsonrpc's request method interface contract so I'm going to fudge there.

let workDoneProgress = new Progress<WorkDone>(workDone => { displayUI(workDone.Percent); });
let partialResultProgress = new Progress<PartialSymbolsResult>(partial => { addToResultsList(partial); });
let finalResult = await rpc.invokeAsync('lookupSymbols', { 
    entry: 'workspa', // this is one of the regular parameters for the method
    workDone: workDoneProgress, 
    partialResult: partialResultProgress
  },
  cancellationToken);

The invokeAsync method of the json-rpc library (or whatever/however it's called) can detect that two named arguments have values that conform to the Progress<T> type, and serialize that via the spec described in the description of this issue. When callbacks to $/progress come in, they are matched by the progress token to the right Progress<T> object and the value of the callback is passed directly to the Progress<T> callback. So you see, it's all strongly typed. The only method that takes all progress type data structures is the $/progress handler, but that is a general handler that merely dispatches to a callback based on the progress token. So you have unique methods with type-awareness everywhere you actually process the progress data.

And yes, in the LSP spec you'll probably want to describe it as you've done, but perhaps link to a reference spec that describes progress so that you don't have to. But for the server it seems pretty simple: to send progress updates just send a notification to $/progress with a progressToken matching either workDone.__jsonrpc__progressToken or partialResult.__jsonrpc__progressToken and the value to transmit. There's no extra state to store or matching to do.

Compared to passing in bool's and leaving the server to find the request message's "id" which may not be visible to them as it's hidden in the JSON-RPC library they're using, I think this is actually easier to implement and even to describe.

And if the server's JSON-RPC library happened to be progress aware, it becomes even easier, as they can simply expect the workDone and partialResult values to be null or an instance of Progress<T> (or their platform-equivalent) and they can simply call report on that object.

from vs-streamjsonrpc.

(note, I edited-in the last paragraph to my last comment after posting the comment, so if you're reading email, please take care to notice it).

As you also expressed a concern regarding the implementation of the jsonrpc library, I'd like to address that. Is your concern mostly on the client or the server side?

from vs-streamjsonrpc.

Actually I was not very clear in my last comment. Even worse I mixed arguments of my initial approach with the one for the generic approach, which made things worse. So let me try to clarify this:

1. I was not trying to argue that this can't be strongly typed in TS. I fully agree it can. I was trying to say that the interpretation on the protocol level and therefore the LSP spec needs to refer to these data structures, property names and may be positional parameters.
2. in the model that is currently discussed (and partly proposed) in LSP the work done progress and the partial result progress are handled completely different which makes it easier to explain them (and I think even easier to implement in a protocol library, not the json-rpc library) since it is more explicit.

Let me detail this a little:

1. partial result is bound to a request and therefore the request id is used. This should therefore best be handled in the library and exposed with a nice interface. My thinking was that the library would provide such a token for all requests. So it is easy to implement and easy to sepc. Whether that token can be used is something to be decided on the application level. The library supports generally to send these notifications as the library generally support cancellation although not all requests can be canceled. Since whether the partial progress token should be used is decided by the application layer so every protocol (or even every method) can have different means to encode this information. All that needs to go into the spec is the $/progress notification sent from the server to the client with an indication on each specific request specification whether it supports such a partial progress reporting.
2. work done progress is on the application (LSP) layer and not in json rpc. This is why I talked about a token before. This is currently not a $ method. It is window/progress/report with its own token so to speak. This would only be handled in the vscode-languageserver-protocol package.

I think specifying this with a generic approach that covers both cases is absolutely doable but IMO harder to understand since it is more complex to spec.

I either need to inspect properties or use positional arguments. Inspecting properties and converting them to a progress type inside a literal seems wired to me. How deep would the library inspect properties. Would this only happen on the first param?. So implementing this in a json-rpc library looks strange to me.

So what is left are positional parameters. So what I can do for LSP is specing that if there is a second parameter it is partial progress and if there is a third parameter there is a work done progress using the tagging interface you propose. We nowhere in the LSP spec have functionality / semantic bound to param positions. This is why I think this is harder to understand on a LSP spec level.

I also think that positional parameters are harder to evolve. This is in general not an issue if you have a typed language with method signature overloads. But on the protocol level all there is are JSON literals and positions. So you either need to specify a new method name or add any additional parameters to the end (this is by the way why LSP uses one param literal; it is extremely easy to evolve. And we might want to evolve the work done progress in the future).

So my bottom line is: can this be nicely implemented in a JSON RPC library using the proposed tagging interface for positional parameters. Definitely YES. Will this help with specifying a protocol and evolving it. There I have my doubts.

from vs-streamjsonrpc.

I spend a little time on thinking how I would spec and implement that in a more general way in jsonrpc and LSP and here is an idea (which would not be like this without the valuable discussion in this issue):

• spec a $/progress notification and add support to vscode-jsonrpc to send such a notification. The notification can either be bound to a request id or can have another token. There will be no param matching using a tagging interface. So like a CannellationToken there will be a generic progress token which support sending a $/progress notification.
• concrete progress for both partial results and work done is speced on the application layer. How the receiver of a request detects that progress reporting is possible is up to the app layer to specify. Could be a special tagging interface, a property on a literal, a positional parameter, ....

The reason for this approach is that a tagging interface alone doesn't help to convey the type of progress. The jsonrpc layer can only convert a tagging interface into Progress<any> and not into Progress<WorkDone> or Progress<PartialResult>. Doing this concrete conversion still requires some specification in terms of param position, property name, .... (see my last comment) and some code in the application layer. Since code in the application layer is need anyways we might do the detection there as well.

Here are a couple of concrete examples:

Using a special token for both partial progress and work done. In LSP the param literal could look like this:

{
    workDone: 15, // Use token 15
    partialProgress: 27 // User token 27
}

If the request id is used for partial progress this could look like this:

{
    workDone: 15, // Use token 15
    partialProgress: true
}

The app layer would convert workDone and the partialProgress properties in Progress<WorkDone> and Progress<PartialProgess> objects which encapsulate the low level communication based on the generic progress token in the json-rpc lib.

from vs-streamjsonrpc.

The reason for this approach is that a tagging interface alone doesn't help to convey the type of progress. The jsonrpc layer can only convert a tagging interface into Progress<any> and not into Progress<WorkDone> or Progress<PartialResult>

In .NET this isn't a problem because the JSON-RPC library can see that the target method to be invoked has a Progress<T> parameter type and thus discover what T is.
In TypeScript, I didn't expect this to be a problem either because at the JSON-RPC library level, you can just new up a Progress<any> and pass it to the registered handler (which accepts a Progress<WorkDone>) and everything Just Works, does it not?

Your most recent proposal looks similar to mine, except that instead of the progress arguments being JSON objects with a specially recognized property name whose value is the token, your argument value is simply the token. That, as you say, requires the application-level to recognize and apply the special treatment of "this is a progress token". Either the application must then translate this to the more useful, native and JSON-RPC agnostic Progress<T> type, or the application must register specific calls and parameter names/positions with the JSON-RPC library so that it knows how to do it.

Our .NET JSON-RPC library could do that too (and without registration) by recognizing that the target method takes a Progress<T> rather than an integer|string that comes in over the wire and decide to interpret the argument value as a progress token, but that makes overload resolution more complicated and springs up ambiguity if a JSON-RPC target object defines two overloads with signatures such as:

void MyMethod(int arg1);
void MyMethod(Progress<WorkDone> arg1);

Now when "MyMethod" is called with an integer, which overload should be selected? In your proposal it would be ambiguous because all we have for the argument is an integer which could be interpreted two ways. We might simply "define" that when there is ambiguity, we prefer the closest match (in this case, take the int overload).

So I guess I could live with your proposal here. But it seems like it's making your own job harder (at the application level at least, if not also in the library) by requiring registration or manual handling of the token. And given I'm still confused by your Progress<any> comment, let's perhaps resolve that before finalizing on a solution.

from vs-streamjsonrpc.

In TypeScript, I didn't expect this to be a problem either because at the JSON-RPC library level, you can just new up a Progress and pass it to the registered handler (which accepts a Progress) and everything Just Works, does it not?

It is about typing and creating the right instance (the Progress comment). The typing we could may be fix using some kind of type inference as we do with the RequestType and NotificationType. However I want to ensure that we can instantiate a more specific type for the work progress and for the partial result process. For the work process I want make sure we do some batching and make sure that we only send a notification every ~100ms. For the partial progress I want to implement something that keeps track of the chunks send so that the receiver can find out if something goes wrong I want to encapsulate this in the corresponding progress classes.

Either the application must then translate this to the more useful, native and JSON-RPC agnostic Progress type, or the application must register specific calls and parameter names/positions with the JSON-RPC library so that it knows how to do it.

Agree, but from an abstraction point of view there is not difference between the two. In both cases (at least in TS where as not reflection) I need to tell the system what progress type to instantiate. We could look into decorations but this still means write code.

Our .NET JSON-RPC library could do that too (and without registration) by recognizing that the target method takes a Progress rather than an integer|string that comes in over the wire and decide to interpret the argument value as a progress token, but that makes overload resolution more complicated and springs up ambiguity if a JSON-RPC target object defines two overloads with signatures such as

I think you could even instantiate a concrete type (e.g. PartialProgress). Right?. If we had a tagging interface Would you do that matching even for properties in parameters or only for positional top level params?.

I also find it a little wired if the library does this for progress and not for other types. Or do you have some smart conversion code as well that converts a JSON object into a specific object if there is a special constructor on that class. I do that in a layer we maintain in the application code to shield users from interpreting these JSON strcutures.

I do see now that a tagging interface helps with languages that have reflection. But TS doesn't so this in one way or the other has to be handled in the application layer either be registering converters in the JSON-RPC library or by handling it in the app layer. So far in LSP all did all this in the app layer.

I am out for a public holiday tomorrow. I will think about it a little more.

from vs-streamjsonrpc.

However I want to ensure that we can instantiate a more specific type for the work progress and for the partial result process.

I didn't think "instantiating a more specific type" mattered. In .NET every single value has a type, but in Javascript both of these types would just be object from Javascript's perspective, would it not? I thought the type instantiated was just a TypeScript concept, and one that a simple declarative cast to type Progress<T> from Progress<any> would be just as good as a strongly typed instantiation of Progress<WorkDone> in the first place. What am I missing?

For the work process I want make sure we do some batching and make sure that we only send a notification every ~100ms. For the partial progress I want to implement something that keeps track of the chunks send so that the receiver can find out if something goes wrong I want to encapsulate this in the corresponding progress classes.

That all sounds highly app-specific. I think it's a worthy goal, and one that the protocol shouldn't prohibit, but I don't think the protocol should be in the business of actively helping achieve that either.

I think you could even instantiate a concrete type (e.g. PartialProgress). Right?

Well, we'd have to know how to recognize the parameter type as a special Progress related one so that we could apply the custom deserialization handling. The pattern in .NET is that a method takes IProgress<T> and the caller gets to decide what implementation to supply, so I think that's the only thing we'd bother supporting. The purpose of the argument is to give the method a way to callback. It's not to assist the method in throttling down how many callbacks are made. That's the job of the method itself, and it could employ a wrapper around the IProgress if that was useful to it.

If we had a tagging interface Would you do that matching even for properties in parameters or only for positional top level params?

I'm only imagining special progress treatment for whole arguments -- not deep inspection of argument values to see if we can find patterns inside them.

I also find it a little wired if the library does this for progress and not for other types. Or do you have some smart conversion code as well that converts a JSON object into a specific object if there is a special constructor on that class. I do that in a layer we maintain in the application code to shield users from interpreting these JSON strcutures.

In .NET, virtually all serializers include built-in support for .NET primitives and then require that other complex objects define their own serialization schema in one way or another. With newtonsoft.json and .NET's DataContractSerializer, members can be decorated as whether they should be serialized or not. When deserializing, the outer type T is usually given to the deserialize method, and from that T the deserializer infers all the JSON members that will be read, and what types they will be deserialized to.
Folks in .NET usually do not deal with "JSON literals" because the strong-typed experience of deserializing to "POCOs" is far superior.

So yes, StreamJsonRpc makes a habit of deserializing each JSON-RPC argument to the type that the method parameter expects to receive. Progress<T> is not a serializable type, and it will require special handling in order to make it serializable, just like we've done with CancellationToken.

I do see now that a tagging interface helps with languages that have reflection. But TS doesn't so this in one way or the other has to be handled in the application layer either be registering converters in the JSON-RPC library or by handling it in the app layer. So far in LSP all did all this in the app layer.

Maybe this goes back to the Progress<any> cast to Progress<T> discussion we're having, but I don't see why this has to be any harder for TypeScript. I'll try to whip up an example to show it off.

from vs-streamjsonrpc.

OK, regarding the Progress<any> to Progress<T> conversion in TypeScript, I've confirmed it works without any issue here:

https://github.com/AArnott/ProgressTypeScriptSample/blob/master/index.ts

Feel free to clone and run locally. Does that help?

from vs-streamjsonrpc.

I am not at all saying that a generic Progress<T> can't be implemented in TS. It of course can as you showed and as we do in other situations where a single standard implementation meets most use cases like the CancellationToken.

What I am trying to get to is the following:

• in most cases such a generic Progress<T> is not what the application layer wants to talk to. It wants to talk to a more specific (not only typing wise but also implementation wise) Progress<WorkDone> and Progress<PartialResult>. At least for LSP. So someone needs to instantiate those and I agree it should be the app layer.
• if the app layer should to do this conversion why have a generic tagging interface in the first place. Then the app layer can do the conversion on a normal property like (e.g. the workDone: string | number) instead of a Progress<any> / tagging interface.
• I still have a hard time saying that the json-rpc layer should inspect (deeply) each property on a JSON literal to check whether the value has the progress tagging shape and if so convert it into a Progress<T>. So such a support will only work with positional params. Or if we support plugging in a converter into the JSON-RPC layer. Such a converter however must come from the app layer which makes it complexity wise the same as doing the conversion on the app layer (although it would still be a cool thing to have in a generic way, not limited to progress).

So I guess my question is: will this tagging interface cause more complexity (in terms of explaining it and documenting it) then it helps on the implementation side?

Regarding your example: what I would like to do is folding DoMyBidding into the WorkDoneProgress. Something like (I just typed it in here. It is not correct since it is missing the timer functionality but I think it conveys the idea.)

interface WorkDone {
   precent: number;
}

class WorkDoneProgress extends Progress<WorkDone> {

	private lastSent: number;

	report(value: WorkDone) {
        let current = (new Date()).getTime();
		if (this.lastSent === undefined || current - this.lastSent > 100 || value.percentage >== 100) {
            super.report(value);
            this.lastSent = current;
        }
    }
}

from vs-streamjsonrpc.

@dbaeumer, I've updated the issue description to simplify the progress token from a JSON object that conforms to an interface to any legal JSON token. null is specially recognized as an indication that the client does not want progress updates. Where possible of course the entire argument could be omitted from the request to signify the same thing.

Does this look good?

from vs-streamjsonrpc.

@AArnott I does look good to me. However I would like to add the following two things:

• an id field to the param literal of the $/progress notification. This will be the value of the request id. I would like to add this since progress is bound to a request and it makes correlating these messages easier (in a log for example) and may be detect if a $/progress message is received for a request that is already closed. I would still use a token property to correlate the progress
• do we need to specify in which scope the progressToken needs to be unique. I see the following options:
  • globally unique
  • request unique

from vs-streamjsonrpc.

Thanks for the feedback. I've clarified that the progress token is expected to be unique to the session.

Correlating the progress notification with the request for logging purposes sounds useful. I wouldn't want the client library to decide to drop "stale" progress for a completed request however in case that progress includes partial results that the caller is still expecting. The client that minted the progress token and also has access to the request id would already have the ability to correlate the two (if it chose), putting the onus on the server to specify both is an extra burden that I wonder if it's worth adding. Especially considering the spec thus far allows for the server to implement this protocol without any special json-rpc library support, but if the notification must be sent with the request id, no server could implement it without library support or the library exposing the request id in some way.

This spec isn't incompatible with progress notification responses to requests without an id (i.e. requests from the client that are notifications themselves). We could debate whether a client that wants progress updates should be sending a notification instead of a request/response style message in the first place, but I'm just saying if we required request ID to be included in the progress callback, we would be closing the door on progress on notifications.

We could document that a server MAY include a request_id parameter with its $/progress notification for the client's use in correlating/logging, but that it isn't required. But going back to the fact that I think that's polluting the "app layer" with a protocol-layer request id, I'd rather avoid this altogether.

from vs-streamjsonrpc.

Sure. I've updated the spec.

from vs-streamjsonrpc.