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

Improvements & tweaks of the RPC system #12368

Closed
tortmayr opened this issue Apr 2, 2023 · 3 comments
Closed

Improvements & tweaks of the RPC system #12368

tortmayr opened this issue Apr 2, 2023 · 3 comments
Assignees
Labels
core issues related to the core of the application messaging issues related to messaging

Comments

@tortmayr
Copy link
Contributor

tortmayr commented Apr 2, 2023

The new RPC system has been in test now for a quite a while and some minor issues have
been identified that could be fixed or tweaked:

Issues:

  • Pending requests are not properly rejected when the services are reloaded (either due to manual refresh or temporary connection loss) (see also Dirty editors cannot be saved after application recovers from "offline" mode #12195)
  • The NotificationMessage type requires a id property. However, this property does not serve any purpose and could be removed (it is only needed to match request-responses but not for notifications)

API improvements:

  • The RpcMessageEncoder/Decoder interfaces could be reworked. In fact they are generic object encoders and not necessarily have to be used to encode/decode RPC messages.
  • The JsonRpcProxyFactory could be improved to work with arbitrary rpc-protocol implementations. Instead of directly referencing the Theia RpcProtocol type it could use a generic wrapper interface for sending requests & notifications. This would make it easier to use the Proxy infrastructure with other protocols like vscode-json-rpc or gRPC.
  • Make use of the deferred util type in the JsonRpcProxyFactory

Documentation:

  • The jsdoc for JsonRpcProxyFactory and related types has not been updated an is still referencing the old vscode-jsonrpc protocol. Ideally we would also rename and drop to Rpc* (e.g. RpcProxyFactory) because the underlying protocol is no longer json-rpc. Rpc is more generic and will also work if we decide to change the underlying protocol in the future.
  • The website json-rpc documentation should also be double checked to ensure that it`s up to date
@tortmayr tortmayr added messaging issues related to messaging core issues related to the core of the application labels Apr 2, 2023
@tortmayr tortmayr self-assigned this Apr 2, 2023
@tsmaeder
Copy link
Contributor

tsmaeder commented Apr 5, 2023

The RpcMessageEncoder/Decoder interfaces could be reworked.

How would you rework them? Aren't they exactly capturing the mapping of rpc messages to general objects?

The JsonRpcProxyFactory could be improved to work with arbitrary rpc-protocol implementations

What's the goal here? I'd rather not introduce an abstraction if we don't abstract over at least two cases.

@tortmayr
Copy link
Contributor Author

tortmayr commented Apr 5, 2023

The JsonRpcProxyFactory could be improved to work with arbitrary rpc-protocol implementations

What's the goal here? I'd rather not introduce an abstraction if we don't abstract over at least two cases.

The goal here is to enable easy reuse of the proxy functionality in combination with other rpc protocol implementations.
A example that we have encountered a couple of times (for instance in GLSP but also in closed-source projects) is rpc communication with an external server via vscode-jsonrpc. In this case the Theia backend just serves as bridge for tunneling frontend requests to the server connection and vice versa. (Something that was supported with the old protocol out of the box because it was also jsonrpc-based see also #11405)
Currently the JsonRpcProxyFactory is hardscoped to the Theia RpcProtocol. This means to support the described pattern adopters have to manually convert Theia RPC messages to vscode-jsonrpc messages (and vice versa) before forwarding them in the backend.

The JsonRpcProxyFactory in theory is already implemented in a way to support this. It uses a Factory to create the actual PpcProtocol on top of the channel. The only problem is that we currently don't have an interface for the RpcProtocol and instead are always directly using the default implementation class. If an adopter now wants to use vscode-jsonrpc it would be required to subclass RPCProtocol and override/customize most of its functionality.

In fact the ProxyFactory just uses the sendRequest and sendNotification method so an easy non-intrusive way to support arbitary protocol implementations would be the introduction of an `RpcServer interface:

interface RpcServer{
    sendRequest<T>(method: string, args: any[]): Promise<T> 
    sendNotification(method: string, args: any[]): void
}

that is implemented by the RpcProtocol class and used in the proxy factory.

The RpcMessageEncoder/Decoder interfaces could be reworked.

How would you rework them? Aren't they exactly capturing the mapping of rpc messages to general objects?

Yes they are. However, one could argue that is mapping is the responsibility of the RpcProtocol and the enocder/decoders should just be generic encoders capable of handling any (serializable) JS object. I have no hard opinion on this but in my opinion the RPC message creation should be handled by the RpcProtocol itself.

@tortmayr
Copy link
Contributor Author

@tsmaeder After further working on this issue and reconsideration I agree with your initial statement and I have removed the items in question from the bullet list.
While I still think it would be nice to enable reuse of the proxy infrastructure for arbitrary protocols it is currently not easily doable without creating a unnecessary complex abstraction for this edge case.

tortmayr added a commit to eclipse-theia/theia-website that referenced this issue 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 issue 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 issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application messaging issues related to messaging
Projects
None yet
Development

No branches or pull requests

2 participants