Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor RPCProtocol #8972

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

tsmaeder
Copy link
Contributor

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

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
Contributor 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
Contributor Author

tsmaeder commented Mar 4, 2021

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

Signed-off-by: Thomas Mäder <tmader@redhat.com>
@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