-
Notifications
You must be signed in to change notification settings - Fork 114
Add unmanaged comms in supervisor API #9109
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
base: main
Are you sure you want to change the base?
Conversation
E2E Tests 🚀 |
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.
Comments from pair review with @DavisVaughan
}); | ||
} | ||
|
||
export async function supervisorApi(): Promise<PositronSupervisorApi> { |
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.
From review with Davis:
We might be able to make activate()
async, then await
the supervisor API, then set a global SUPERVISOR_API
. Then supervisorApi()
no longer needs to be async, which would make it much more ergonomic:
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.
Would allow us to export error classes and use instanceof
to match them, without contagious asyncness.
5df1d7d
to
c16042b
Compare
7de3f44
to
62dc899
Compare
00ea64b
to
ece8b0e
Compare
And fix a few issues
To avoid down-casting
Easier to match on from other extensions
62dc899
to
0a1d724
Compare
* Channel receiver (rx). Async-iterable to receive values from the channel. | ||
*/ | ||
export class Receiver<T> implements AsyncIterable<T>, AsyncIterator<T>, vscode.Disposable { | ||
private i = 0; |
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.
Nit: Better name for this var?
} | ||
|
||
/** | ||
* Channel receiver (rx). Async-iterable to receive values from the channel. |
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.
Modeling the channel receiver as an async iterable is a cool idea!
|
||
/** | ||
* This set of type definitions defines the interfaces used by the Positron | ||
* Positron Supervisor extension. |
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.
Hah, thanks for catching this!
async dispose(): Promise<void> { | ||
this.close(); | ||
for (const disposable of this.disposables) { | ||
disposable.dispose(); |
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.
Nit: clear array of disposables after they are disposed, to clear references and make this idempotent
Progress towards #9094.
Progress towards #7448.
This PR adds support for "unmanaged" comms, i.e. comms that are not mapped to Positron clients. Instead they are fully managed by the kernel supervisor.
No longer need to call
registerClientInstance()
implemented in Fix/suppress log noise around comms #2892 to tell the core to ignore the unknown client.The comm is fully private to the extension that creates it. It can send and receive messages of any type.
The API makes it easy to receive and handle messages in order via a new
Channel
type that works like Rust channels. It also makes it easy to send requests and notifications.Like Positron clients, the message structure is based on JSON-RPC messages embedded in the jupyter comm messages. But we're moving towards a stricter version of JSON-RPC with includion of an
id
field, in order to support notifications from frontend to backend as the field disambiguates with requests. See Support backend-side events/notifications in OpenRPC contracts #7448.Here is how it works:
The extension is the only side that can create an unmanaged comm. To do so it calls:
The promise resolves to an instance of
Comm
:Channels:
Implemented by the supervisor in
Channel.ts
, they provide a similar abstraction to Rust channels. This makes it easy to handle messages in order. When the channel closes, this is like a close event instructing the extension code to clean up the comm.The Comm wraps a
ReceiverChannel
, the receiving side. The sending side is owned by the supervisor.Messages are a tagged union of Notification | Request. The
Request
type includes ahandle()
method that must be called with a closure. Throwing from the closure rejects the request. The following is a minimal implementation of a receiver loop:There is a bit of boilerplate with this approach but it offers a lot of flexibility. The extension can break the loop at any time or call third-party handlers for messages (which we do for DAP, see below).
Lifecycle:
If
dispose()
is called on a frontend Comm, the receiver channel is closed and drained and acomm_close
message is sent to the backend.If the backend closes the comm, the channel is closed. That serves as an event notifying the extension that the comm is closed and should be disposed.
Sending messages:
For ergonomics, messages are not sent from the extension with a channel but by calling
notify()
andrequest()
methods on the comm. Both can throw if channel is closed.request()
can simply be awaited until the response comes in. In addition toCommClosedError
, the caller should handleCommRpcError
.DAP and Server comms:
The Server comm is a light abstraction over a comm. Any comm can be a server comm if it follows the handshaking protocol. Call
createServerComm()
instead ofcreateComm()
. When this resolves, you'll have a fully started server comm whose backend side is ready to receive connections.The DAP comm is a light wrapper around a
Comm
. It's created withcreateDapComm()
(which internally callscreateServerComm()
) and returns aDapComm
. The point of this wrapper if to gain access toDapComm::handleMessage()
. This method can be called from the extension receiver loop to handle all or a subset of our "standard" DAP messages. For instance it can handlestart_debug
messages to start a debugging session from the backend. The positron-r extension fully delegates to this handle method, but extensions are free to handle more message types, or to override handling of some of the standard DAP messages.Frontend notifications:
Ark needed a way determine whether a message (from either a client or unmanaged comm) is a request or a notification. Up to now it assumed all messages are requests. The simplest way to achieve this is to follow the JSON-RPC standard and use presence of an
id
field as discriminant.This required changes in the core. The base comm now generates an
id
field for requests, andperformRpcWithBuffers()
make sure the field is present if the caller did not include one.Progress toward Support backend-side events/notifications in OpenRPC contracts #7448.
Miscellaneous changes:
Dap is no longer a Positron Client. In the future, LSP server comms should also become unmanaged comms instead of Positron clients, but I thought we'd start with DAP to lower the stakes in case something goes wrong with the refactor.
Added a
justfile
in the supervisor source folder. Calljust
to regenerate allpositron-supervisor.d.ts
files in the monorepo, reformatted if needed.Renamed the
Comm
supervisor type toClient
. So that unmanaged comms are nowComm
.Positron-r now creates the "Ark Comm". This can be used in the future for private communication with Ark. For the time being, this is used in unit tests for comm messaging, using the new infrastructure for extension tests developed in Fix
OpenEditor
event on Windows (and test it) #8816.Release Notes
New Features
Bug Fixes
QA Notes
Full suite running at https://github.com/posit-dev/positron/actions/runs/17293101714