-
Notifications
You must be signed in to change notification settings - Fork 5
feat(remote-comms): Add explicit connection management for intentional disconnects #699
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,14 @@ export class PlatformServicesClient implements PlatformServices { | |
| return new PlatformServicesClient(stream, logger); | ||
| } | ||
|
|
||
| /** | ||
| * Launch a new worker with a specific vat id. | ||
| * | ||
| * @param vatId - The vat id of the worker to launch. | ||
| * @param vatConfig - The configuration for the worker. | ||
| * @returns A promise for a duplex stream connected to the worker | ||
| * which rejects if a worker with the given vat id already exists. | ||
| */ | ||
| async launch( | ||
| vatId: VatId, | ||
| vatConfig: VatConfig, | ||
|
|
@@ -138,14 +146,37 @@ export class PlatformServicesClient implements PlatformServices { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Terminate a worker identified by its vat id. | ||
| * | ||
| * @param vatId - The vat id of the worker to terminate. | ||
| * @returns A promise that resolves when the worker has terminated | ||
| * or rejects if that worker does not exist. | ||
| */ | ||
| async terminate(vatId: VatId): Promise<void> { | ||
| await this.#rpcClient.call('terminate', { vatId }); | ||
| } | ||
|
|
||
| /** | ||
| * Terminate all workers managed by the service. | ||
| * | ||
| * @returns A promise that resolves after all workers have terminated | ||
| * or rejects if there was an error during termination. | ||
| */ | ||
| async terminateAll(): Promise<void> { | ||
| await this.#rpcClient.call('terminateAll', []); | ||
| } | ||
|
|
||
| /** | ||
| * Initialize network communications. | ||
| * | ||
| * @param keySeed - The seed for generating this kernel's secret key. | ||
| * @param knownRelays - Array of the peerIDs of relay nodes that can be used to listen for incoming | ||
| * connections from other kernels. | ||
| * @param remoteMessageHandler - A handler function to receive remote messages. | ||
| * @returns A promise that resolves once network access has been established | ||
| * or rejects if there is some problem doing so. | ||
| */ | ||
| async initializeRemoteComms( | ||
| keySeed: string, | ||
| knownRelays: string[], | ||
|
|
@@ -158,10 +189,24 @@ export class PlatformServicesClient implements PlatformServices { | |
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Stop network communications. | ||
| * | ||
| * @returns A promise that resolves when network access has been stopped | ||
| * or rejects if there is some problem doing so. | ||
| */ | ||
| async stopRemoteComms(): Promise<void> { | ||
| await this.#rpcClient.call('stopRemoteComms', []); | ||
| } | ||
|
|
||
| /** | ||
| * Send a remote message to a peer. | ||
| * | ||
| * @param to - The peer ID to send the message to. | ||
| * @param message - The message to send. | ||
| * @param hints - Optional hints for the message. | ||
| * @returns A promise that resolves when the message has been sent. | ||
| */ | ||
| async sendRemoteMessage( | ||
| to: string, | ||
| message: string, | ||
|
|
@@ -170,20 +215,62 @@ export class PlatformServicesClient implements PlatformServices { | |
| await this.#rpcClient.call('sendRemoteMessage', { to, message, hints }); | ||
| } | ||
|
|
||
| /** | ||
| * Explicitly close a connection to a peer. | ||
| * Marks the peer as intentionally closed to prevent automatic reconnection. | ||
| * | ||
| * @param peerId - The peer ID to close the connection for. | ||
| * @returns A promise that resolves when the connection is closed. | ||
| */ | ||
| async closeConnection(peerId: string): Promise<void> { | ||
| await this.#rpcClient.call('closeConnection', { peerId }); | ||
| } | ||
|
|
||
| /** | ||
| * Manually reconnect to a peer after intentional close. | ||
| * Clears the intentional close flag and initiates reconnection. | ||
| * | ||
| * @param peerId - The peer ID to reconnect to. | ||
| * @param hints - Optional hints for reconnection. | ||
| * @returns A promise that resolves when reconnection is initiated. | ||
| */ | ||
| async reconnectPeer(peerId: string, hints: string[] = []): Promise<void> { | ||
| await this.#rpcClient.call('reconnectPeer', { peerId, hints }); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it considered reconnection (as one would do after a network outage) or is it just a new connection to the same endpoint as before? In particular, do we expect clist entries to survive a trip through manual disconnect/manual reconnect?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is a reconnection not a new connection. It reuses the existing RemoteHandle and remote endpoint. Clist entries and other endpoint state persist across the disconnect/reconnect cycle. |
||
|
|
||
| /** | ||
| * Handle a remote message from a peer. | ||
| * | ||
| * @param from - The peer ID that sent the message. | ||
| * @param message - The message received. | ||
| * @returns A promise that resolves with the reply message, or an empty string if no reply is needed. | ||
| */ | ||
| async #remoteDeliver(from: string, message: string): Promise<string> { | ||
| if (this.#remoteMessageHandler) { | ||
| return await this.#remoteMessageHandler(from, message); | ||
| } | ||
| throw Error(`remote message handler not set`); | ||
| } | ||
|
|
||
| /** | ||
| * Send a message to the server. | ||
| * | ||
| * @param payload - The message to send. | ||
| * @returns A promise that resolves when the message has been sent. | ||
| */ | ||
| async #sendMessage(payload: JsonRpcMessage): Promise<void> { | ||
| await this.#stream.write({ | ||
| payload, | ||
| transfer: [], | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Handle a message from the server. | ||
| * | ||
| * @param event - The message event. | ||
| * @returns A promise that resolves when the message has been sent. | ||
| */ | ||
| async #handleMessage(event: MessageEvent<JsonRpcMessage>): Promise<void> { | ||
| if (isJsonRpcResponse(event.data)) { | ||
| const { id } = event.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.
Thank you for adding these doc comments. It's interesting that our lint rules demand jsdoc comments on functions but not on methods. I wonder if we can change that (or did it actually change and that's what motivated these?)
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.
We follow the MetaMask eslint rule which doesn't demand jsdoc comments on methods. We certainly can enable it though