feat(extension): Move Kernel instance to kernel-worker #141

grypez · 2024-10-10T15:59:17Z

Changes

`@ocap/kernel`

Adds the InitKernel command, which for now is intended to be used only as a reply. The reply contains information about how long the kernel took to initialize, as well as the id of its default vat.

`@ocap/extension`

Moves the kernel types in kernel-worker to a separate file.
Moves the Kernel instance into the kernel-worker file.
The kernel-worker sends an InitKernel reply to offscreen once it has finished setting up.

packages/extension/src/offscreen.ts

rekmarks

Overall a great step forward!

packages/extension/src/kernel-worker.ts

rekmarks · 2024-10-10T20:12:02Z

packages/extension/src/kernel-worker.ts

      await handleKernelCommand(message);
    } else {
-      console.error('Received unexpected message', message);
+      logger.debug(`Received unexpected message ${stringify(message)}`);


When do we expect to hit this now, and why do we downgrade it to a debug log?

Missed this from the previous approach I was taking. We don't expect to hit it, and it should be an error.

More broadly I have been thinking about what log levels we should use when, and I think in the case where we expect to sometimes receive and ignore unhandleable messages the right log level is info or log, and the string .debug( ought to be more or less uncommitable to our codebase.

Yeah, I think logs should be either error (as necessary, when undefined / invalid / unexpected states occur), info (sparingly, to provide feedback for actions), or probably not be committed. warn can useful for third parties, which isn't relevant to us just yet.

rekmarks · 2024-10-10T20:30:54Z

packages/extension/src/kernel-worker.ts

+  await reply({
+    method: KernelCommandMethod.InitKernel,
+    params: { defaultVat: defaultVatId, initTime: performance.now() - start },
+  });


The introduction of the InitKernel method creates an unfortunate asymmetry in the kernel commands and our notion of "replies" from the kernel to the background ("console"). We probably will want to establish a notion of unprompted messages ("notifications"?) from the kernel to the console, but I think it would be better if we deferred that until later. We don't need this method to exist now.

If by 'this method' you mean the unprompted messages you referred to, I agree we don't need them now. But I am using the InitKernel method to prevent the offscreen from sending messages to the kernel before it is ready to receive them, so some form of init message is necessary for this PR.

Hm, we shouldn't need to do that. Specifically, it should be a concern of the streams. In other words, by the time the offscreen has a reference to the kernel worker and can send it messages, the kernel worker should be ready to receive them. Right now, there would be interval where messages could theoretically be sent before the kernel worker's stream is initialized, so we can leave it in for now. However, I will try to fundamentally solve the issue by modifying the message channel protocol following #142.

packages/extension/src/kernel-worker.ts

rekmarks · 2024-10-10T21:31:55Z

packages/extension/src/offscreen.ts

-        await handleKernelCommand(message);
+        isKernelCommand(message)
+          ? await kernelWorker.sendMessage(message)
+          : logger.debug('Received unexpected message', message);


Should this also be an error?

I don't think so. The background stream passes any Json and may reasonably receive messages which are not KernelCommands. It should be an info though.

It may do so in the future if we decide to make it so, but for now, in this context, I think it's fine to let it be an error. In the production setting, there'll be a different connection entirely in place here.

rekmarks · 2024-10-10T21:57:24Z

I agree it would be reasonable to decide that different messages can be sent over the streams. However, we don't intend for that to be the case right now, and it's therefore an error state: #146

rekmarks

LGTM!

grypez changed the base branch from main to grypez/abstract-message-channel-init October 10, 2024 15:59

grypez mentioned this pull request Oct 10, 2024

Do Not Merge: Move kernel to dedicated worker #123

Closed

6 tasks

grypez force-pushed the grypez/abstract-message-channel-init branch from 373bdbe to dc78c18 Compare October 10, 2024 17:43

grypez force-pushed the grypez/move-kernel-to-dedicated-worker-with-message-channel branch from d3f155a to 724d2c6 Compare October 10, 2024 17:44

grypez force-pushed the grypez/abstract-message-channel-init branch 2 times, most recently from 732004d to 773957f Compare October 10, 2024 18:02

Base automatically changed from grypez/abstract-message-channel-init to main October 10, 2024 18:06

grypez force-pushed the grypez/move-kernel-to-dedicated-worker-with-message-channel branch from 724d2c6 to cb6e33f Compare October 10, 2024 18:08

grypez marked this pull request as ready for review October 10, 2024 18:10

grypez requested a review from a team as a code owner October 10, 2024 18:10

grypez force-pushed the grypez/move-kernel-to-dedicated-worker-with-message-channel branch from cb6e33f to 70e43cc Compare October 10, 2024 18:37

grypez changed the title ~~Move kernel to dedicated worker with message channel~~ feat(extension): Move kernel to dedicated worker Oct 10, 2024

grypez changed the title ~~feat(extension): Move kernel to dedicated worker~~ feat(extension): Move Kernel instance to kernel-worker Oct 10, 2024

rekmarks reviewed Oct 10, 2024

View reviewed changes

packages/extension/src/offscreen.ts Show resolved Hide resolved

rekmarks requested changes Oct 10, 2024

View reviewed changes

rekmarks force-pushed the grypez/move-kernel-to-dedicated-worker-with-message-channel branch from 8d57666 to 4113bec Compare October 10, 2024 21:29

rekmarks reviewed Oct 10, 2024

View reviewed changes

packages/extension/src/kernel-worker.ts Outdated Show resolved Hide resolved

rekmarks reviewed Oct 10, 2024

View reviewed changes

rekmarks approved these changes Oct 10, 2024

View reviewed changes

grypez enabled auto-merge (squash) October 10, 2024 22:35

grypez and others added 9 commits October 10, 2024 18:35

feat(kernel): Add InitKernel command.

8764332

feat(extension): Use init kernel command.

90d63ee

refactor(extension): Extract kernel types from kernel-worker.

737d6bf

refactor(extension): Move kernel into kernel-worker.

5b133e6

Update offscreen.ts (#144)

916474f

tidy up

de34ebb

feat(extension): Restore error logs in message handlers (#146)

a9734da

guard drain handler

523a948

refactor(extension): Appease the gremlin in the CI linter

cc274b8

grypez force-pushed the grypez/move-kernel-to-dedicated-worker-with-message-channel branch from 217cb81 to cc274b8 Compare October 10, 2024 22:36

grypez merged commit 033ea62 into main Oct 10, 2024

grypez deleted the grypez/move-kernel-to-dedicated-worker-with-message-channel branch October 10, 2024 22:38

This was referenced Nov 4, 2024

fix: Add Source Filtering to ChromeRuntimeReader for Multi-Endpoint Support #215

Merged

Establish console / logger method convention. #217

Closed

feat(extension): Move Kernel instance to kernel-worker #141

feat(extension): Move Kernel instance to kernel-worker #141

Uh oh!

Conversation

grypez commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

@ocap/kernel

@ocap/extension

Uh oh!

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekmarks commented Oct 10, 2024

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grypez commented Oct 10, 2024 •

edited

Loading

`@ocap/kernel`

`@ocap/extension`