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

core: improve & refactor rpc protocol #12581

Merged
merged 1 commit into from
Jun 7, 2023
Merged

core: improve & refactor rpc protocol #12581

merged 1 commit into from
Jun 7, 2023

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented May 30, 2023

What it does

Implements the tweaks and fixes for the rpc system discussed in #12368

  • Ensure that pending requests are properly rejected when the underlying service channel is closed.
  • Remove obsolete id-property from NotificationMessages. Ids are only required fro matching request-response pairs.
  • Use a deferred in the RpcProxyFactory for protocol initialization

Contributed on behalf of STMicroelectronics

How to test

Nothing to test in particular. The change should not break any functionality. Everything should work as before

Review checklist

Fixes #12581

Reminder for reviewers

tortmayr added a commit that referenced this pull request May 30, 2023
- Ensure that pending requests are properly rejected when the underlying service channel is closed.
- Remove obsolete id-property from `NotificationMessage`s. Ids are only required fro matching request-response pairs.
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
- Use a deferred in the `RpcProxyFactory` for protocol initialization

Fixes #12581
tortmayr added a commit to eclipse-theia/theia-website that referenced this pull request May 30, 2023
Updates the outdated documentation for setting up RPC services by
- Removing any notion of the old vscode json-rpc protocol. The neutral term RPC is used instead
- Replacing the outdated loggingServer example with a current version of the taskServer
- Removing sections of the documentation that are now obsolete because the underlying framework code has been reworked

Note: he code snippets are based on eclipse-theia/theia#12581.
So this PR is blocked until  the  referenced PR is approved and merged
(the last section 
Part of eclipse-theia/theia#12368
tortmayr added a commit to eclipse-theia/theia-website that referenced this pull request May 30, 2023
Updates the outdated documentation for setting up RPC services by
- Removing any notion of the old vscode json-rpc protocol. The neutral term RPC is used instead
- Replacing the outdated loggingServer example with a current version of the taskServer
- Removing sections of the documentation that are now obsolete because the underlying framework code has been reworked

Note: he code snippets are based on eclipse-theia/theia#12581.
So this PR is blocked until  the  referenced PR is approved and merged
(the last section 
Part of eclipse-theia/theia#12368
@tortmayr tortmayr requested a review from tsmaeder May 31, 2023 06:08
Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

A couple of comments inline. In general, it might have been easier to make the renames a separate, non-functional PR. Right now the renames hide the functional changes a bit.

@@ -111,7 +110,7 @@ export interface RpcMessageDecoder {
export interface RpcMessageEncoder {
cancel(buf: WriteBuffer, requestId: number): void;

notification(buf: WriteBuffer, requestId: number, method: string, args: any[]): void
notification(buf: WriteBuffer, method: string, args: any[]): void
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the request id has a use as an ordering tool: it allows us to determine the sequence of messages sent from a particular RPC endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I opted for making the notification id optional. This way we can still use it for tracing with the base protocol.
Adopters that use a custom protocol or reuse the RpcMessageEncoder can simply omit the id if they don't need.

this.onDidCloseConnectionEmitter.fire(undefined);
// Wait for connection in case the backend reconnects
this.waitForConnection();
});
protocol.channel.onError(err => {
if (this.rpcDeferred.state !== 'resolved') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does Deferred do if the state is resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the lines in question. They were committed on accident (rebasing shenanigans)

packages/core/src/common/messaging/proxy-factory.ts Outdated Show resolved Hide resolved
);
this.connectionPromise.then(connection => {
connection.channel.onClose(() => {
this.rpcDeferred.promise.then(protocol => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not exactly the same as before: waitForConnection can be called more than once, but we a single Deferred instance can only be resolved once. I think we have to allocate a new instance here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

const defaultRPCConnectionFactory: RpcConnectionFactory = (channel, requestHandler) => new RpcProtocol(channel, requestHandler);
const defaultRpcProtocolFactory: RpcProtocolFactory = (channel, requestHandler) => new RpcProtocol(channel, requestHandler);

export interface RpcProxyFactoryOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

This interface seems unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼

packages/core/src/node/logger-backend-module.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added jsonrpc issues related to jsonrpc messaging issues related to messaging labels May 31, 2023
- Ensure that pending requests are properly rejected when the underlying service channel is closed.
- Make  id-property from `NotificationMessage`s.  optional. Ids in notification are not strictly required and can be safely omitted.
- Rename `RpcConnectionFactory` to `RpcProtocolFactory` 
- Use a deferred in the `RpcProxyFactory` for protocol initialization

Part of #12581
@tortmayr
Copy link
Contributor Author

tortmayr commented Jun 1, 2023

In general, it might have been easier to make the renames a separate, non-functional PR. Right now the renames hide the functional changes a bit.

Yeah that a good point. I have addressed your review and pushed an update that removes the renames and only contains the functional changes.

I will open a separate PR for the renaming changes.

@tortmayr tortmayr requested a review from tsmaeder June 1, 2023 09:56
tortmayr added a commit that referenced this pull request Jun 1, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
tortmayr added a commit that referenced this pull request Jun 1, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
tortmayr added a commit that referenced this pull request Jun 1, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
@tsmaeder
Copy link
Contributor

tsmaeder commented Jun 7, 2023

@tortmayr I checked out the latest state of the branch and for me, it is quite broken: plugins do not start at all. There is nothing obviously wrong when looking at the logs.

@tsmaeder
Copy link
Contributor

tsmaeder commented Jun 7, 2023

@tortmayr something might be wrong with my system. Hold on before you start panicking ;-)

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

Looks good, now.

@tsmaeder tsmaeder merged commit c42adc2 into master Jun 7, 2023
@github-actions github-actions bot added this to the 1.39.0 milestone Jun 7, 2023
tortmayr added a commit that referenced this pull request Jun 8, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
tortmayr added a commit that referenced this pull request Jun 20, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
tsmaeder pushed a commit that referenced this pull request Jun 22, 2023
- Rename `JsonRpcProxyFactory` and related types to `RpcProxyFactory` (Rpc*). The naming scheme was a remainder of the old vscode jsonr-rpc based protocol.
  By simply using the `Rpc` suffix the class names are less misleading and protocol agnostic.
- Keep deprecated declarations of the old `JsonRpc*` namespace. The components are heavily used by adopters so we should maintain this deprecated symbols for a while to enable graceful migration without hard API breaks.
Naturally this is open for discussion, but judging from the files I had to touch in the core framework alone I think it's definitely 
a good idea to give adopters a grace period.

Complementary website PR: eclipse-theia/theia-website#422

Fixes #12581
Contributed on behalf of STMicroelectronics
tortmayr added a commit to eclipse-theia/theia-website that referenced this pull request Jul 26, 2023
* Update RPC documentation

Updates the outdated documentation for setting up RPC services by
- Removing any notion of the old vscode json-rpc protocol. The neutral term RPC is used instead
- Replacing the outdated loggingServer example with a current version of the taskServer
- Removing sections of the documentation that are now obsolete because the underlying framework code has been reworked

Note: he code snippets are based on eclipse-theia/theia#12581.
So this PR is blocked until  the  referenced PR is approved and merged
(the last section 
Part of eclipse-theia/theia#12368
@tortmayr tortmayr deleted the tortmayr/12368 branch April 29, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jsonrpc issues related to jsonrpc messaging issues related to messaging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants