Skip to content

Conversation

@grypez
Copy link
Contributor

@grypez grypez commented Oct 4, 2024

Changes

@ocap/kernel

  • The VatWorker type is relocated to @ocap/extension as an implementation-specific detail.
  • Adds a VatWorkerService interface intended for initializing and deleting vat workers and delivering a connected duplex stream to the kernel.
  • Kernel now accepts a VatWorkerService in its constructor, and Kernel.launchVat no longer accepts a VatWorker.

@ocap/extension

  • The VatWorker type is relocated from @ocap/utils to vat-worker-service.ts.
  • VatWorker.init now promises a port and an object instead of a stream pair and an object.
  • Adds a VatWorkerServer and VatWorkerClient in offscreen.ts which communicate over a message channel. The client will be moved into kernel-worker.ts along with the kernel in a subsequent PR.

@ocap/utils

  • Handled callback is relocated from @ocap/extension.

@grypez grypez force-pushed the grypez/differentiate-command-hierarchy branch from 6af86d7 to a0bd613 Compare October 6, 2024 21:17
@grypez grypez force-pushed the grypez/vat-worker-service branch 4 times, most recently from 20174a4 to 4d1014c Compare October 7, 2024 15:04
@grypez grypez force-pushed the grypez/differentiate-command-hierarchy branch 4 times, most recently from cca8901 to d146904 Compare October 7, 2024 16:54
@grypez grypez force-pushed the grypez/vat-worker-service branch from 4d1014c to 234f242 Compare October 7, 2024 17:26
Base automatically changed from grypez/differentiate-command-hierarchy to main October 8, 2024 04:31
@grypez grypez force-pushed the grypez/vat-worker-service branch from 234f242 to 6964fff Compare October 8, 2024 19:28
@grypez grypez marked this pull request as ready for review October 8, 2024 19:37
@grypez grypez requested a review from a team as a code owner October 8, 2024 19:37
Note: although this move removes all references to @ocap/test-utils
from @ocap/extension, the tsconfig entry is maintained because
inserting a delay() is useful while developing in the extension.
@grypez grypez force-pushed the grypez/vat-worker-service branch from 6964fff to 08c61b6 Compare October 8, 2024 22:24
@grypez grypez force-pushed the grypez/vat-worker-service branch from 08c61b6 to e0e9195 Compare October 9, 2024 02:33
sirtimid
sirtimid previously approved these changes Oct 9, 2024
Copy link
Contributor

@sirtimid sirtimid left a comment

Choose a reason for hiding this comment

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

LGTM! 🙂

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

This enshrines browser APIs (MessagePort, postMessage) within the kernel package. We of course need to have concrete implementations that use these APIs, but the kernel should expect a platform-agnostic interface. This doesn't need to happen in this PR, but I'm requesting changes so we can verify consensus on the matter.

@grypez
Copy link
Contributor Author

grypez commented Oct 9, 2024

This enshrines browser APIs (MessagePort, postMessage) within the kernel package. We of course need to have concrete implementations that use these APIs, but the kernel should expect a platform-agnostic interface. This doesn't need to happen in this PR, but I'm requesting changes so we can verify consensus on the matter.

Indeed; what other platforms are we targeting?

I'll note that the interface for VatWorkerClient.initWorker is the platform agnostic VatId => DuplexStream, though the implementation is browser specific.

The friction is that in the browser case we need to make the port on the server end and send it to the client end before wrapping it in a stream, and that this transport is not supported by our Json bound streams. If we have an abstract VatWorkerServer and VatWorkerClient then we might dissolve the VatWorker type and directly initialize and destroy the realm in the server implementation. In principle realm management and connection transit are separate problems but I think in practice they will be coupled.

@rekmarks
Copy link
Member

rekmarks commented Oct 9, 2024

Indeed; what other platforms are we targeting?

In the long view, we should be able to drop kernel components into Node.js without issue. For more purposes closer to home, we may well have to use transport mechanisms other than MessagePort in when we get to mobile and React Native.

In principle realm management and connection transit are separate problems but I think in practice they will be coupled.

I agree, my point is merely that only the vat worker service implementations should be aware of this coupling. My concern would be assuaged if we had types (possibly base classes if it makes sense) named VatWorkerServer and VatWorkerClient that make no reference to platform-specific APIs, and we make the kernel accept those types instead.

@grypez
Copy link
Contributor Author

grypez commented Oct 9, 2024

My concern would be assuaged if we had types (possibly base classes if it makes sense) named VatWorkerServer and VatWorkerClient that make no reference to platform-specific APIs, and we make the kernel accept those types instead.

Alright, I will abstract the interface to a type, relocate the current implementations to @ocap/extension, and rename them to ExtensionVatWorker[Client|Server]. I will also split the client and server unit tests into their own files in @ocap/extension.

I don't think I will attempt an abstract class until we have another implementation case.

@grypez grypez force-pushed the grypez/vat-worker-service branch 2 times, most recently from 93048fe to 6a83983 Compare October 9, 2024 19:22
@grypez grypez force-pushed the grypez/vat-worker-service branch from 6a83983 to b3709ae Compare October 9, 2024 19:26
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM, with the caveat that when the kernel and worker service client are moved into their own worker, we should use the initializeMessageChannel / receiveMessagePort functions from the streams package. Moreover, establishing the communications channel is perhaps more properly the concern of the offscreen.ts / iframe.ts, and it would be great if we could abstract away the ports from the worker service classes entirely.

Comment on lines +19 to +22
initWorker: (
vatId: VatId,
) => Promise<DuplexStream<StreamEnvelopeReply, StreamEnvelope>>;
deleteWorker: (vatId: VatId) => Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't think "init" and "delete" are the right verbs here. "launch" and "terminate" would be more appropriate:

Suggested change
initWorker: (
vatId: VatId,
) => Promise<DuplexStream<StreamEnvelopeReply, StreamEnvelope>>;
deleteWorker: (vatId: VatId) => Promise<void>;
launchWorker: (
vatId: VatId,
) => Promise<DuplexStream<StreamEnvelopeReply, StreamEnvelope>>;
terminateWorker: (vatId: VatId) => Promise<void>;

Arguably, restating "worker" in the method names is also unnecessary:

Suggested change
initWorker: (
vatId: VatId,
) => Promise<DuplexStream<StreamEnvelopeReply, StreamEnvelope>>;
deleteWorker: (vatId: VatId) => Promise<void>;
launch: (
vatId: VatId,
) => Promise<DuplexStream<StreamEnvelopeReply, StreamEnvelope>>;
terminate: (vatId: VatId) => Promise<void>;

@grypez
Copy link
Contributor Author

grypez commented Oct 9, 2024

when the kernel and worker service client are moved into their own worker, we should use the initializeMessageChannel / receiveMessagePort functions from the streams package.

I intend that they will communicate with the offscreen document over webworker.postMessage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants