Skip to content

Refactor RPCProtocol#8972

Merged
tsmaeder merged 1 commit intoeclipse-theia:masterfrom
tsmaeder:16502_refactor_rpcprotocol
Apr 23, 2021
Merged

Refactor RPCProtocol#8972
tsmaeder merged 1 commit intoeclipse-theia:masterfrom
tsmaeder:16502_refactor_rpcprotocol

Conversation

@tsmaeder
Copy link
Member

What it does

This refactoring clarifies an decouples a bunch of things. This is a pure refactoring which should not change behaviour.

  1. Each connection to a plugin host has an explicit host id: we already have this concept in HostedPluginSupport#initRpc, but instead of using the already known host id, we use the id of the first plugin deployed on the plugin host for a connection id.
  2. JsonPlugin(Client|Server) now explicitly reference a plugin host id instead of encoding that in the messages being transferred
  3. MessageConnection only handle strings now: we're sending json messages, so this is not really a restriction.
  4. RPCMultiplexer now is an implementation detail of RPCProtocol.
  5. RPCProtocol only deals with sending remote invocations over a connection: all things specific to plugin hosts and routing have been removed.

How to test

Run Theia with with at least a back-end and one front-end plugin. No exceptions related to rpc processing should appear in neither the back-end nor front-end logs.
Note: front-end plugins still don't work with electron.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added the jsonrpc issues related to jsonrpc label Jan 20, 2021
@ericwill ericwill mentioned this pull request Jan 20, 2021
33 tasks
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

I don't fully understand the system, but so far those changes are looking good to me.

@tsmaeder
Copy link
Member Author

@marechal-p thx for the review.

@benoitf
Copy link
Contributor

benoitf commented Jan 27, 2021

Hello, I still don't get the real benefits, (clarifies is subjective there) except maybe introducing new bugs ?

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I don't see regression using Theia

@paul-marechal
Copy link
Member

@tsmaeder merge at will.

@tsmaeder
Copy link
Member Author

tsmaeder commented Mar 4, 2021

@marechal-p Not merging because of downstream reasons 🤷 .

@tsmaeder tsmaeder force-pushed the 16502_refactor_rpcprotocol branch from 72dd264 to 1025a94 Compare March 10, 2021 14:22
@tsmaeder tsmaeder force-pushed the 16502_refactor_rpcprotocol branch from 1025a94 to 5128136 Compare March 19, 2021 15:43
@tsmaeder tsmaeder force-pushed the 16502_refactor_rpcprotocol branch from 5128136 to 8b93e69 Compare April 8, 2021 11:46
@tsmaeder tsmaeder force-pushed the 16502_refactor_rpcprotocol branch from 8b93e69 to d44b0a4 Compare April 23, 2021 08:05
Signed-off-by: Thomas Mäder <tmader@redhat.com>
@tsmaeder tsmaeder force-pushed the 16502_refactor_rpcprotocol branch from d44b0a4 to dbf86d2 Compare April 23, 2021 11:48
@tsmaeder tsmaeder merged commit c5d440a into eclipse-theia:master Apr 23, 2021
@vince-fugnitto vince-fugnitto added this to the 1.13.0 milestone Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jsonrpc issues related to jsonrpc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants