-
Notifications
You must be signed in to change notification settings - Fork 4
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
[WIP] Plugin message-rpc #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. I added some minor comments.
const sender = event.sender; | ||
try { | ||
// Get the multiplexer for a given window id | ||
const windowChannelData = this.windowChannelMultiplexer.get(sender.id)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a !
if we check in the next line whether windowChannelData exists?
const onCloseEmitter = new Emitter<ChannelCloseEvent>(); | ||
const onMessageEmitter = new Emitter<MessageProvider>(); | ||
const onErrorEmitter = new Emitter<unknown>(); | ||
const pipe = childProcess.stdio[4] as Writable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be as Readable
as we are reading data from it below?
private cancel(req: string): string { | ||
return `{"type":${MessageType.Cancel},"id":"${req}"}`; | ||
this.registerEncoder(ObjectType.RESPONSE_ERROR, { | ||
is: () => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we add an encoder that never applies?
} | ||
|
||
export function encodeMessageStart(buffer: ArrayBuffer): Uint8Array { | ||
// Prepend message length before data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is no longer what we do here. remove comment
({ | ||
range: Converter.toRange(change.range), | ||
rangeOffset: change.rangeOffset, | ||
rangeLength: change.rangeLength, | ||
text: change.text | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed here so the file should not have changed, shouldn't it?
@@ -35,7 +35,7 @@ export class NotificationExtImpl implements NotificationExt { | |||
): Promise<R> { | |||
const source = new CancellationTokenSource(); | |||
const id = new Deferred<string>(); | |||
const progress = task({ report: async item => this.proxy.$updateProgress(await id.promise, item)}, source.token); | |||
const progress = task({ report: async item => this.proxy.$updateProgress(await id.promise, item) }, source.token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed here so the file should not have changed, shouldn't it?
import { es5ClassCompat } from '../common/types'; | ||
import { ObjectsTransferrer } from '../common/rpc-protocol'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed here so the file should not have changed, shouldn't it?
The commit upgrades the `yarn.lock` file in preparation for the `v1.25.0` release. Upgrading the lockfile is important to get a better representation of what downstream applications pull. Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
ce98274
to
89ad62d
Compare
…eia#11095) Fixes a bug where `when` contexts and custom context keys were ignored.
Fixes a bug preventing Monaco from rendering keyboard shortcut hints correctly.
* [playwright] Improve getting started documentation Requires eclipse-theia/theia-playwright-template#1 Fixes eclipse-theia#11015 Change-Id: I5838de12b7e3e840e354e8230a35c752c9f25dd1 Signed-off-by: Philip Langer <planger@eclipsesource.com>
The commit updates the root `package.json` to use the proper `id` for builtin themes so they are not re-downloaded by the `extension-pack`. Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also eclipse-theia#10781). The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data. Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc). This has major impacts on the Messaging API as we no longer expose a `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead. - Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc) - Replace usage of `vscode-ws-jsonrpc` with a custom binary RPC protocol. - Refactor all connection providers to use the new binary protocol. - Ensure that the `RemoteFileSystemProvider` API uses `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead of unnecessarily converting from and to `Uint8Arrays`. - Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol. - Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding. - Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`. The plugin API still uses its own RPC protocol implementation. Currently we have to encode/decode between binary data to handle RPC calls from a plugin context. Aligning the two protocols and zero-copy tunneling of RPC messages is planned for a follow-up PR. Contributed on behalf of STMicroelectronics. Closes eclipse-theia#10684
Address review feedback: - Merge and RpcClient and RpcServer in RpcProtocol - Only export public API in index.ts - Remove outdated comments - Refactor error value encoder - Fix casing in `MessageTypes Integrate performance improvements - Use Uint8Array instead of ArrayBuffer when feasible - Use TextEncoder.encodeInto() instead of encode() - Use improved encoding for length values - Preallocate a single TextEncoder - Consider potential buffer byteOffsets when creating new Write/Reader buffers. Contributed on behalf of STMicroelectronics
+ Remove depdendency to vscode-ws-jsonrpc Contributed on behalf of STMicroelectronics
Refactor plugin-ext rpc protocol to reuse the new binary message-rpc protocol. - - Remove custom RPC message encoding and handling reuse message-rpc -- Implement `QueuingChannelMultiplexer`that queues messages and sends them accumlated on the next process.tick (replaces the old `Multiplexer` implementation -- Refactors proxy handlers and remote target handlers -- Use `Channel` instead of `MessageConnection` for creating new instances of `RPCProtocol` -- Implement special message encoders and decoders for the plugin communication - Adapt the `HostedPluginServer` and `HostedPluginClient` API to send/receive messages in binary format instead of strings. - Implement a message protocol for process pipe streams to be able to send messages in binary format between the hosted-plugin-process and the plugin-host. Co-authored-by: Lucas Koehler <lkoehler@eclipsesource.com> Part of eclipse-theia#10684 Contributed on behalf of STMicroelectronics
89ad62d
to
e931411
Compare
- add missing decoder/encoder for range, uri, vscode uri, binary buffer - cache messages in hosted plugin watcher before listeners are registered. otherwise channel open messages can be lost
- NotificationExt was instantiated in the main context - There were unused notification proxy identifiers for main and ext in the wrong contexts
Treat them as requests to support cancellation
Code integrated in merged upstream PR eclipse-theia#11261 |
Review and adapt prompt templates
Refactor plugin-ext rpc protocol to reuse the new binary message-rpc protocol.
QueuingChannelMultiplexer
that queues messages and sends them accumlated on the next process.tick (replaces the oldMultiplexer
implementationChannel
instead ofMessageConnection
for creating new instances ofRPCProtocol
Adapt the
HostedPluginServer
andHostedPluginClient
API to send/receive messages in binary format instead of strings.Implement a message protocol for process pipe streams to be able to send messages in binary format between the hosted-plugin-process and the plugin-host.
Contributed on behalf of STMicroelectronics
Part of eclipse-theia#10684