Update JSON-RPC #10265

colin-grant-work · 2021-10-12T22:23:40Z

What it does

Fixes #10264 by switching to use a fork of vscode-ws-jsonrpc that has been updated to support vscode-jsonrpc@6.

I could use some help, though, as I'm not very familiar with all of the details of the JSON RPC system. I'll make some comments pointing to the places where I'm uncertain of the right move.

How to test

Since this modifies the core of our communication with the backend, thoroughly test... everything, I guess.
In particular, check that interactions with plugins work as expected.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

packages/core/src/common/messaging/proxy-factory.spec.ts

packages/core/src/common/messaging/proxy-factory.ts

packages/core/src/node/messaging/ipc-connection-provider.ts

packages/plugin-ext/src/common/connection.ts

packages/plugin-ext/src/common/plugin-message-reader.ts

packages/plugin-ext/src/common/plugin-message-writer.ts

paul-marechal

Those changes go in the right direction, but I think we should rather look into replacing vscode-ws-jsonrpc by a in-repo alternative that we can maintain. This should simplify our consumption of vscode-jsonrpc at the same time. cc @eclipse-theia/core

tsmaeder · 2021-10-14T11:37:17Z

I would tend toward in-sourcing this code: I believe the newest version of vscode-jsonrpc actually has support for sockets/ipc, etc. We would be better served maintaining the bit of glue code needed ourselves.
Using a library introduces a certain transaction cost (updating, providing PR's, etc.) that needs to be offset by savings on writing/maintaining the code in-project. Those savings are too small, IMO.

tsmaeder

Code looks good, in general, but need to test.

packages/core/package.json

packages/core/src/common/messaging/proxy-factory.ts

paul-marechal · 2021-10-15T14:10:46Z

I went ahead and pushed commits that replace occurrences of vscode-languageserver-types with vscode-languageserver-protocol. The latter already depends on the former and re-exports everything from it. Since the typings are coupled between those two packages, instead of trying to depend on the -types ourselves, making sure we have the same version as -protocol, we can just consume -protocol alone.

colin-grant-work · 2021-10-15T17:39:02Z

@paul-marechal, would you to / like me to see about removing some of the code copied from vscode-ws-jsonrpc / monaco-jsonrpc in favor of functionality drawn directly vscode-jsonrpc, or are you happy with the current state of this PR as an improvement on the existing situation?

paul-marechal · 2021-10-15T22:38:13Z

Rebased on master. I tried to untangle what I could with regards to our logical channels over websockets, as well as this weird BareMessageConnection (previously IConnection).

IIUC IConnection is a remnant of when we dealt with LSP directly. We needed something to route JSON-RPC messages from websockets to process pipes. I don't like the IConnection name as it is not as generic as it sounds. It is closer to a MessageConnection but without the helper methods like sendRequest or sendNotification, hence the new name BareMessageConnection. Feedback welcome on the naming.

From there, I simply made helper methods to get both MessageConnection and BareMessageConnection from a Channel. Once this was done, wiring our WebSocketChannel to JSON-RPC worked right away.

I don't think I looked at everything @colin-grant-work touched, as I only focused on the core messaging systems.

No longer depending on vscode-ws-jsonrpc.

msujew

I started testing this PR. Most of it looks good, however I can't seem to start a debug session with the vscode-js-debug extension. The debug session does not start in the frontend and I get the following error:

Uncaught (in promise) SyntaxError: Unexpected token o in JSON at position 1 
at JSON.parse (<anonymous>)
at PluginDebugAdapterSession.write (/workspace/theia/packages/plugin-ext/lib/plugin/node/debug/plugin-debug-adapter-session.js:67:52)

packages/core/src/common/messaging/proxy-factory.ts

packages/core/src/common/messaging/bare-message-connection.ts

packages/core/src/common/messaging/web-socket-channel.ts

packages/core/src/common/messaging/channel.ts

packages/core/src/common/messaging/proxy-factory.ts

paul-marechal · 2021-10-19T19:54:28Z

@msujew good catch, this is now fixed. There is still an error being thrown when stopping the session but it already happens on master so that makes it out of scope for this PR.

msujew

I couldn't find any more regressions introduced by the changes. Besides the one remark the code looks good to me as well.

Anyway, I'll let others review/test this as well before actually approving.

packages/core/src/common/messaging/channel.ts

paul-marechal · 2021-10-22T00:27:43Z

@colin-grant-work @tsmaeder I rebased and finalized those changes.

@tsmaeder looking at all this code made me conclude that something is off regarding the way messaging is handled with plugins... Mostly I don't really understand the use of vscode-jsonrpc's APIs in order to not actually make JSON-RPC go through it. It was really awkward to read and follow. The biggest smells (that I noticed) being the following places:

theia/packages/plugin-ext/src/common/plugin-message.ts

Lines 19 to 28 in 41f377c

    
           export interface PluginMessage extends Message { 
        
               /** 
        
                * Bogus JSON-RPC version because we don't actually implement JSON-RPC here. 
        
                */ 
        
               jsonrpc: '0.0' 
        
               /** 
        
                * Actual string payload being transmitted. 
        
                */ 
        
               content: string 
        
           }

theia/packages/plugin-ext/src/common/connection.ts

Lines 66 to 76 in 41f377c

    
           send(content: string): void { 
        
               // vscode-jsonrpc's MessageReader/Writer expect to send JSON-RPC messages. 
        
               // Use a bogus jsonrpc version and pass along the `content` to send. 
        
               // `content` here is opaque: it could be any string. 
        
               const message: PluginMessage = { jsonrpc: '0.0', content }; 
        
               this.connection.writer.write(message); 
        
           } 
        
           onMessage(cb: (data: string) => void): void { 
        
               this.connection.reader.listen((message: PluginMessage) => cb(message.content)); 
        
           }

It really feels like we are shoehorning this custom protocol down the poor vscode-jsonrpc APIs...

Please tell me if I misunderstood anything.

colin-grant-work · 2021-11-02T22:45:58Z

@tsmaeder, it seems like there's some overlap between the work here and the work you've been doing on the debug system. Would you mind taking a look at this so we can merge if appropriate, and you can then use some of the new messaging equipment in your PR?

paul-marechal · 2021-11-03T15:58:26Z

packages/core/src/common/messaging/proxy-factory.ts

+            // Extract the cancellation token if it is the last argument passed to the method to call over JSON-RPC.
+            let token: CancellationToken | undefined;
+            if (CancellationToken.is(args[args.length - 1])) {
+                token = args.pop();
+            }
+            // Convert the list of arguments into a structure that allows us to conserve undefined values.
+            const params: any[] = this.createJsonRpcParameters(args);
+            // Push back the token if there was one.
+            // `vscode-jsonrpc` will remove the `CancellationToken` out of `params` when is is the last element.
+            if (token) {
+                params.push(token);
+            }


We could get rid of this logic if we assumed nobody should design their APIs over RPC using undefined params.

The createJsonRpcParameters method is a mechanism to properly propagate undefined parameters over JSON-RPC, because undefined values cannot be transmitted verbatim over JSON.

e.g. JSON.stringify(['a', undefined, 2]) returns '["a",null,2]'

To me it's not really clear what the motivation for these here changes are: is this fixing a known bug? If yes, which one?

Like I mentioned, undefined values are transformed into null without the type system knowing.

theia/packages/plugin-ext/src/main/common/plugin-paths-protocol.ts

Lines 24 to 25 in 8bf15dc

/** Returns storage path for given workspace */

getHostStoragePath(workspaceUri: string | undefined, rootUris: string[]): Promise<string | undefined>;

Here workspaceUri is actually string | null when used over RPC. We got "lucky" here as the only implementation checks for truthiness to detect the type rather than some workspaceUri === undefined which would not work.

One solution would be for interfaces meant to be used over RPC to almost never use undefined in their API.

tsmaeder

I really wish we could have done the changing of the imports to 'vscode-languageserver-protocol' in a separate PR: 88 files is very big when "real changes" are insterspersed with just changed imports.

The only real change I'm requesting is the renaming of TheiaMessagingConnection: I just find the name very confusing. There are many things in this area of code that I disapprove of, but they were there before so 🤷 .

In the long run, I would like to get rid of jsonrpc anyway: we already have a remote proxy invocation framework in our code and I don't think we need two: the only thing I can see missing is the ability to read partial messages, but that could quite simply be retrofitted.

tsmaeder · 2021-11-05T10:34:31Z

packages/core/src/common/messaging/theia-message-connection.ts

+ * This is close to `vscode_jsonrpc.MessageConnection` but without all the JSON-RPC
+ * specific methods such as `sendRequest`, `sendNotification`, etc.
+ */
+export interface TheiaMessageConnection extends Disposable {


The name of this interface suggests that it is a special kind of MessageConnection, which is simply not the case. This makes it hard to understand the related code.

Why not just simply keep it as IConnection? It's just as non-descriptive, but at least not misleading ;-)

Since its job is to forward messages, how about RPCMessageRelay?

packages/core/package.json

tsmaeder · 2021-11-05T12:38:26Z

packages/core/src/common/messaging/proxy-factory.ts

+            // Extract the cancellation token if it is the last argument passed to the method to call over JSON-RPC.
+            let token: CancellationToken | undefined;
+            if (CancellationToken.is(args[args.length - 1])) {
+                token = args.pop();
+            }
+            // Convert the list of arguments into a structure that allows us to conserve undefined values.
+            const params: any[] = this.createJsonRpcParameters(args);
+            // Push back the token if there was one.
+            // `vscode-jsonrpc` will remove the `CancellationToken` out of `params` when is is the last element.
+            if (token) {
+                params.push(token);
+            }


To me it's not really clear what the motivation for these here changes are: is this fixing a known bug? If yes, which one?

tsmaeder · 2021-11-05T12:44:04Z

packages/plugin-ext/src/common/connection.ts

+ *
+ * @deprecated since 1.19.0
+ */
+export const PluginWebSocketChannel = PluginChannel;


I'm not sure we need a deprecation cycle here: it's not "official" API and I doubt someone actually used this implementation detai.

I'm very fine with that, will remove.

tsmaeder · 2021-11-05T12:48:47Z

packages/plugin-ext/src/common/connection.ts

 */
-export class PluginWebSocketChannel implements IWebSocket {
-    constructor(protected readonly connection: PluginConnection) { }
+export class PluginChannel implements Channel<string> {


We're wrapping a string channel around a json rcp connection which wraps a string channel. That does not really make sense, IMO. I got rid of that all in #10333. So I wouldn't sweat the small stuff here.

I did what I must to get it to compile as before, I'd be more than happy to merge #10333 after this!

packages/plugin-ext/src/common/plugin-message-writer.ts

tsmaeder · 2021-11-05T12:55:20Z

packages/core/src/node/messaging/messaging-contribution.ts

            callback(params, connection);
        });
    }

-    forward(spec: string, callback: (params: MessagingService.PathParams, connection: IConnection) => void): void {
+    forward(spec: string, callback: (params: MessagingService.PathParams, connection: TheiaMessageConnection) => void): void {


Is this even ever used? I didn't find any usages.

Not anymore but despite the amount of changes in this PR I tried to only touch what was broken/bad.

This method is not bothering anyone so I'd keep it for now :)

tsmaeder · 2021-11-05T13:00:37Z

@colin-grant-work do we need more testing on this PR?

colin-grant-work · 2021-11-05T15:59:15Z

@colin-grant-work do we need more testing on this PR?

@tsmaeder, more testing never hurts. It seems that @msujew was happy with his last run through, and I haven't noticed any problems, but you may be aware of use cases that we're not hitting.

tsmaeder · 2021-11-23T16:10:08Z

Btw: does the new version help with #9514?

colin-grant-work · 2021-11-23T16:48:10Z

Btw: does the new version help with #9514?

@tsmaeder, I haven't checked, but it would surprise me if it had much effect on the performance - I don't think there was that much extraneous code running before, and I don't think there's much less now. We still have to JSON-ify everything and send it over the wire (short or long) to the other end. There's a little bit of argument processing on our side and in the package, and that certainly adds a little overhead, but I don't think that's changed much. Someone please bring in a performance tracer and correct me :-).

packages/core/src/common/messaging/theia-message-connection.ts

The rename was done.

msujew · 2021-11-29T12:57:03Z

@colin-grant-work @paul-marechal I assume a rebase is in order due to #10333. I would retest this afterwards and approve if possible. We'd have a few weeks after merging to find possible regressions before the next release.

Updates to latest version of vscode-languageserver-protocol Removes vscode-monaco-jsonrpc dependency Replaces references to vscode-languageserver-types with references to vscode-languageserver-protocol Refactors existing code to match new interfaces Co-authored-by: Paul Maréchal <[email protected] Co-authored-by: Colin Grant <[email protected]>

colin-grant-work · 2022-01-28T16:47:30Z

I'm going to close this for now, since it looks like there's more interest in rewriting our message layer than in updating this package.

colin-grant-work commented Oct 12, 2021

View reviewed changes

packages/core/src/common/messaging/proxy-factory.spec.ts Outdated Show resolved Hide resolved

colin-grant-work commented Oct 12, 2021

View reviewed changes

packages/core/src/common/messaging/proxy-factory.ts Outdated Show resolved Hide resolved

colin-grant-work commented Oct 12, 2021

View reviewed changes

packages/core/src/node/messaging/ipc-connection-provider.ts Outdated Show resolved Hide resolved

colin-grant-work force-pushed the update/jsonrpc branch from 938a78d to 5d5613c Compare October 12, 2021 22:29

colin-grant-work commented Oct 12, 2021

View reviewed changes

colin-grant-work marked this pull request as draft October 12, 2021 22:37

colin-grant-work mentioned this pull request Oct 13, 2021

Update JSON-RPC and Related Dependencies #10264

Closed

colin-grant-work marked this pull request as ready for review October 13, 2021 17:00

paul-marechal previously requested changes Oct 13, 2021

View reviewed changes

colin-grant-work force-pushed the update/jsonrpc branch 2 times, most recently from 058d32c to db3b907 Compare October 14, 2021 23:20

tsmaeder reviewed Oct 15, 2021

View reviewed changes

packages/core/package.json Show resolved Hide resolved

packages/core/src/common/messaging/proxy-factory.ts Outdated Show resolved Hide resolved

paul-marechal force-pushed the update/jsonrpc branch from 67c2e87 to 95f96b4 Compare October 15, 2021 22:18

msujew reviewed Oct 18, 2021

View reviewed changes

paul-marechal reviewed Oct 18, 2021

View reviewed changes

packages/core/src/common/messaging/proxy-factory.ts Outdated Show resolved Hide resolved

colin-grant-work commented Oct 18, 2021

View reviewed changes

paul-marechal force-pushed the update/jsonrpc branch from ccf894b to f83c04a Compare October 19, 2021 19:45

msujew reviewed Oct 20, 2021

View reviewed changes

packages/core/src/common/messaging/channel.ts Outdated Show resolved Hide resolved

paul-marechal force-pushed the update/jsonrpc branch from 55f7b0f to 41f377c Compare October 22, 2021 00:17

paul-marechal force-pushed the update/jsonrpc branch from 79788be to 8d67a60 Compare October 29, 2021 21:57

paul-marechal reviewed Nov 3, 2021

View reviewed changes

colin-grant-work requested a review from tsmaeder November 3, 2021 22:01

vince-fugnitto added dependencies pull requests that update a dependency file jsonrpc issues related to jsonrpc quality issues related to code and application quality labels Nov 5, 2021

tsmaeder previously requested changes Nov 5, 2021

View reviewed changes

paul-marechal force-pushed the update/jsonrpc branch from 8d67a60 to e179450 Compare November 9, 2021 20:11

colin-grant-work force-pushed the update/jsonrpc branch from e179450 to 7bea6ab Compare November 22, 2021 16:12

paul-marechal force-pushed the update/jsonrpc branch from 7bea6ab to 9ed38a1 Compare November 23, 2021 23:20

tsmaeder reviewed Nov 24, 2021

View reviewed changes

packages/core/src/common/messaging/theia-message-connection.ts Outdated Show resolved Hide resolved

colin-grant-work force-pushed the update/jsonrpc branch from 5ab667d to 96a7521 Compare November 30, 2021 19:45

colin-grant-work mentioned this pull request Dec 3, 2021

Remove languageserver-types dependency in favor of -protocol #10500

Merged

1 task

tortmayr mentioned this pull request Jan 28, 2022

Improve performance of JSON-RPC communication #10684

Closed

5 tasks

colin-grant-work closed this Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update JSON-RPC #10265

Update JSON-RPC #10265

colin-grant-work commented Oct 12, 2021 •

edited

Loading

paul-marechal left a comment

tsmaeder commented Oct 14, 2021 •

edited

Loading

tsmaeder left a comment

paul-marechal commented Oct 15, 2021

colin-grant-work commented Oct 15, 2021

paul-marechal commented Oct 15, 2021

msujew left a comment

paul-marechal commented Oct 19, 2021

msujew left a comment

paul-marechal commented Oct 22, 2021 •

edited

Loading

colin-grant-work commented Nov 2, 2021

paul-marechal Nov 3, 2021

tsmaeder Nov 5, 2021

paul-marechal Nov 8, 2021

tsmaeder left a comment

tsmaeder Nov 5, 2021

tsmaeder Nov 5, 2021

colin-grant-work Nov 22, 2021

tsmaeder Nov 5, 2021

tsmaeder Nov 5, 2021

paul-marechal Nov 5, 2021

tsmaeder Nov 5, 2021

paul-marechal Nov 5, 2021 •

edited

Loading

tsmaeder Nov 5, 2021

paul-marechal Nov 5, 2021

tsmaeder commented Nov 5, 2021

colin-grant-work commented Nov 5, 2021

tsmaeder commented Nov 23, 2021

colin-grant-work commented Nov 23, 2021

msujew commented Nov 29, 2021

colin-grant-work commented Jan 28, 2022

	/** Returns storage path for given workspace */
	getHostStoragePath(workspaceUri: string \| undefined, rootUris: string[]): Promise<string \| undefined>;

Update JSON-RPC #10265

Update JSON-RPC #10265

Conversation

colin-grant-work commented Oct 12, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal left a comment

Choose a reason for hiding this comment

tsmaeder commented Oct 14, 2021 • edited Loading

tsmaeder left a comment

Choose a reason for hiding this comment

paul-marechal commented Oct 15, 2021

colin-grant-work commented Oct 15, 2021

paul-marechal commented Oct 15, 2021

msujew left a comment

Choose a reason for hiding this comment

paul-marechal commented Oct 19, 2021

msujew left a comment

Choose a reason for hiding this comment

paul-marechal commented Oct 22, 2021 • edited Loading

colin-grant-work commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder 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

paul-marechal Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Nov 5, 2021

colin-grant-work commented Nov 5, 2021

tsmaeder commented Nov 23, 2021

colin-grant-work commented Nov 23, 2021

msujew commented Nov 29, 2021

colin-grant-work commented Jan 28, 2022

colin-grant-work commented Oct 12, 2021 •

edited

Loading

tsmaeder commented Oct 14, 2021 •

edited

Loading

paul-marechal commented Oct 22, 2021 •

edited

Loading

paul-marechal Nov 5, 2021 •

edited

Loading