Skip to content

Implement kernel storage abstraction layer #180

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.

Sign up for GitHub

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

Merged
FUDCo merged 5 commits into from
Oct 25, 2024
Merged

Implement kernel storage abstraction layer #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f023a4
chore: merge with main branch
FUDCo Oct 18, 2024
214d8ca
feat: Implement kernel storage abstraction layer
FUDCo Oct 21, 2024
1d9b852
chore: get rid of TS whinges in kernel-store test script
FUDCo Oct 22, 2024
8e84a92
chore: clean ups based on review comments; tidying up loose ends that…
FUDCo Oct 23, 2024
ff6f5dc
chore: more post-review cleanup
FUDCo Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/extension/src/kernel-worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { KernelCommand, KernelCommandReply, VatId } from '@ocap/kernel';
import { Kernel } from '@ocap/kernel';
import { MessagePortDuplexStream, receiveMessagePort } from '@ocap/streams';

import { makeKernelStore } from './sqlite-kernel-store.js';
import { makeSQLKVStore } from './sqlite-kv-store.js';
import { ExtensionVatWorkerClient } from './VatWorkerClient.js';

main('v0').catch(console.error);
Expand All @@ -28,10 +28,10 @@ async function main(defaultVatId: VatId): Promise<void> {

// Initialize kernel store.

const kernelStore = await makeKernelStore();
const kvStore = await makeSQLKVStore();

// Create and start kernel.

const kernel = new Kernel(kernelStream, vatWorkerClient, kernelStore);
const kernel = new Kernel(kernelStream, vatWorkerClient, kvStore);
await kernel.init({ defaultVatId });
}
47 changes: 36 additions & 11 deletions ...ages/extension/src/sqlite-kernel-store.ts → packages/extension/src/sqlite-kv-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { KernelStore } from '@ocap/kernel';
import type { KVStore } from '@ocap/kernel';
import { makeLogger } from '@ocap/utils';
import type { Database } from '@sqlite.org/sqlite-wasm';
import sqlite3InitModule from '@sqlite.org/sqlite-wasm';
Expand All @@ -18,14 +18,14 @@ async function initDB(): Promise<Database> {
}

/**
* Makes a {@link KernelStore} for persistent storage.
* Makes a {@link KVStore} for low-level persistent storage.
*
* @param label - A logger prefix label. Defaults to '[sqlite]'.
* @returns The kernel store.
* @returns The key/value store to base the kernel store on.
*/
export async function makeKernelStore(
export async function makeSQLKVStore(
label: string = '[sqlite]',
): Promise<KernelStore> {
): Promise<KVStore> {
const logger = makeLogger(label);
const db = await initDB();

Expand All @@ -44,12 +44,13 @@ export async function makeKernelStore(
`);

/**
* Exercise reading from the database.
* Read a key's value from the database.
*
* @param key - A key to fetch.
* @param required - True if it is an error for the entry not to be there.
* @returns The value at that key.
*/
function kvGet(key: string): string {
function kvGet(key: string, required: boolean): string {
Copy link
Contributor

@sirtimid sirtimid Oct 22, 2024

Choose a reason for hiding this comment

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

Suggested change
function kvGet(key: string, required: boolean): string {
function kvGet(key: string, required: boolean): string | undefined {

But why don't we want to return undefined? I don't get that return undefined as unknown as string; below

Copy link
Member

@rekmarks rekmarks Oct 22, 2024

Choose a reason for hiding this comment

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

Naively, I agree it seems preferable for this function to be honest about occasionally returning undefined.

Copy link
Contributor Author

@FUDCo FUDCo Oct 22, 2024

Choose a reason for hiding this comment

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

This is where TypeScript and I butt heads a lot. Although adding undefined to the possible return types is strictly more correct, "might be undefined" introduces a type contagion that infects distant parts of the call graph, even though in all but a few cases undefined is never an actual possibility and the declaration forces everything that ever touches the return value to implement code to account for the thing that will never happen happening just to make the compiler happy (plus of course all those conditionals cluttering up the code will show up as gaps in code coverage since the thing never happens).

In practice I've found it's sometimes cleaner to pretend to the type system that it's always just a string (or whatever), then just explicitly look for undefined in the few places where that's actually relevant.

Copy link
Member

@rekmarks rekmarks Oct 24, 2024

Choose a reason for hiding this comment

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

If it's not really possible to retrieve an undefined key in practice, why don't we just get rid of getRequired and make get throw if the key is undefined? At the moment, should get ever return undefined, we will get a bunch of hard-to-decipher errors, when we could just get an easily comprehensible one instead.

Copy link
Contributor Author

@FUDCo FUDCo Oct 24, 2024

Choose a reason for hiding this comment

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

It is possible to retrieve an undefined key in practice, and in fact it's useful for testing if a value has been set in the first place (particularly useful in the case of lazy initialization of things that might never be used), so merely having the result be undefined is not itself an error condition. It's only an error when you need the value to be there. In a nutshell, the problem is that TS wants you make an a priori policy decision as to whether undefined is always ok or never ok, but doesn't deal gracefully with the common case where ok vs. not ok is context sensitive.

sqlKVGet.bind([key]);
if (sqlKVGet.step()) {
const result = sqlKVGet.getString(0);
Expand All @@ -60,7 +61,12 @@ export async function makeKernelStore(
}
}
sqlKVGet.reset();
throw Error(`no record matching key '${key}'`);
if (required) {
throw Error(`no record matching key '${key}'`);
} else {
// Sometimes, we really lean on TypeScript's unsoundness
return undefined as unknown as string;
}
}

const sqlKVSet = db.prepare(`
Expand All @@ -70,7 +76,7 @@ export async function makeKernelStore(
`);

/**
* Exercise writing to the database.
* Set the value associated with a key in the database.
*
* @param key - A key to assign.
* @param value - The value to assign to it.
Expand All @@ -82,8 +88,27 @@ export async function makeKernelStore(
sqlKVSet.reset();
}

const sqlKVDelete = db.prepare(`
DELETE FROM kv
WHERE key = ?
`);

/**
* Delete a key from the database.
*
* @param key - The key to remove.
*/
function kvDelete(key: string): void {
logger.debug(`kernel delete '${key}'`);
sqlKVDelete.bind([key]);
sqlKVDelete.step();
sqlKVDelete.reset();
}

return {
kvGet,
kvSet,
get: (key) => kvGet(key, false),
getRequired: (key) => kvGet(key, true),
set: kvSet,
delete: kvDelete,
};
}
32 changes: 16 additions & 16 deletions packages/kernel/src/Kernel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { MessagePortDuplexStream, DuplexStream } from '@ocap/streams';
import type { MockInstance } from 'vitest';
import { describe, it, expect, vi, beforeEach } from 'vitest';

import type { KernelStore } from './kernel-store.js';
import type { KVStore } from './kernel-store.js';
import { Kernel } from './Kernel.js';
import type {
KernelCommand,
Expand All @@ -15,7 +15,7 @@ import type {
import type { StreamEnvelope, StreamEnvelopeReply } from './stream-envelope.js';
import type { VatId, VatWorkerService } from './types.js';
import { Vat } from './Vat.js';
import { makeMapKernelStore } from '../test/storage.js';
import { makeMapKVStore } from '../test/storage.js';

describe('Kernel', () => {
let mockStream: DuplexStream<KernelCommand, KernelCommandReply>;
Expand All @@ -25,7 +25,7 @@ describe('Kernel', () => {
let initMock: MockInstance;
let terminateMock: MockInstance;

let mockKernelStore: KernelStore;
let mockKVStore: KVStore;

beforeEach(() => {
mockStream = {
Expand Down Expand Up @@ -56,23 +56,23 @@ describe('Kernel', () => {
.spyOn(Vat.prototype, 'terminate')
.mockImplementation(vi.fn());

mockKernelStore = makeMapKernelStore();
mockKVStore = makeMapKVStore();
});

describe('getVatIds()', () => {
it('returns an empty array when no vats are added', () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
expect(kernel.getVatIds()).toStrictEqual([]);
});

it('returns the vat IDs after adding a vat', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
expect(kernel.getVatIds()).toStrictEqual(['v0']);
});

it('returns multiple vat IDs after adding multiple vats', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
await kernel.launchVat({ id: 'v1' });
expect(kernel.getVatIds()).toStrictEqual(['v0', 'v1']);
Expand All @@ -81,15 +81,15 @@ describe('Kernel', () => {

describe('launchVat()', () => {
it('adds a vat to the kernel without errors when no vat with the same ID exists', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
expect(initMock).toHaveBeenCalledOnce();
expect(launchWorkerMock).toHaveBeenCalled();
expect(kernel.getVatIds()).toStrictEqual(['v0']);
});

it('throws an error when launching a vat that already exists in the kernel', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
expect(kernel.getVatIds()).toStrictEqual(['v0']);
await expect(
Expand All @@ -103,7 +103,7 @@ describe('Kernel', () => {

describe('deleteVat()', () => {
it('deletes a vat from the kernel without errors when the vat exists', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
expect(kernel.getVatIds()).toStrictEqual(['v0']);
await kernel.deleteVat('v0');
Expand All @@ -113,7 +113,7 @@ describe('Kernel', () => {
});

it('throws an error when deleting a vat that does not exist in the kernel', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
const nonExistentVatId: VatId = 'v9';
await expect(async () =>
kernel.deleteVat(nonExistentVatId),
Expand All @@ -122,7 +122,7 @@ describe('Kernel', () => {
});

it('throws an error when a vat terminate method throws', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
vi.spyOn(Vat.prototype, 'terminate').mockRejectedValueOnce('Test error');
await expect(async () => kernel.deleteVat('v0')).rejects.toThrow(
Expand All @@ -133,7 +133,7 @@ describe('Kernel', () => {

describe('sendMessage()', () => {
it('sends a message to the vat without errors when the vat exists', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
vi.spyOn(Vat.prototype, 'sendMessage').mockResolvedValueOnce('test');
expect(
Expand All @@ -145,15 +145,15 @@ describe('Kernel', () => {
});

it('throws an error when sending a message to the vat that does not exist in the kernel', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
const nonExistentVatId: VatId = 'v9';
await expect(async () =>
kernel.sendMessage(nonExistentVatId, {} as VatCommand['payload']),
).rejects.toThrow(VatNotFoundError);
});

it('throws an error when sending a message to the vat throws', async () => {
const kernel = new Kernel(mockStream, mockWorkerService, mockKernelStore);
const kernel = new Kernel(mockStream, mockWorkerService, mockKVStore);
await kernel.launchVat({ id: 'v0' });
vi.spyOn(Vat.prototype, 'sendMessage').mockRejectedValueOnce('error');
await expect(async () =>
Expand All @@ -165,7 +165,7 @@ describe('Kernel', () => {
describe('constructor()', () => {
it('initializes the kernel without errors', () => {
expect(
async () => new Kernel(mockStream, mockWorkerService, mockKernelStore),
async () => new Kernel(mockStream, mockWorkerService, mockKVStore),
).not.toThrow();
});
});
Expand Down
18 changes: 10 additions & 8 deletions packages/kernel/src/Kernel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { DuplexStream } from '@ocap/streams';
import type { Logger } from '@ocap/utils';
import { makeLogger, stringify } from '@ocap/utils';

import type { KernelStore } from './kernel-store.js';
import type { KVStore } from './kernel-store.js';
import {
isKernelCommand,
KernelCommandMethod,
Expand All @@ -27,7 +27,7 @@ export class Kernel {

readonly #vatWorkerService: VatWorkerService;

readonly #storage: KernelStore;
readonly #storage: KVStore;

// Hopefully removed when we get to n+1 vats.
readonly #defaultVatKit: PromiseKit<Vat>;
Expand All @@ -37,7 +37,7 @@ export class Kernel {
constructor(
stream: DuplexStream<KernelCommand, KernelCommandReply>,
vatWorkerService: VatWorkerService,
storage: KernelStore,
storage: KVStore,
logger?: Logger,
) {
this.#stream = stream;
Expand Down Expand Up @@ -94,10 +94,12 @@ export class Kernel {
break;
case KernelCommandMethod.KVGet: {
try {
const result = this.kvGet(params);
const value = this.kvGet(params);
const result =
typeof value === 'string' ? `"${value}"` : `${value}`;
await this.#reply({
method,
params: result,
params: `~~~ got ${result} ~~~`,
});
} catch (problem) {
// TODO: marshal
Expand Down Expand Up @@ -144,12 +146,12 @@ export class Kernel {
}
}

kvGet(key: string): string {
return this.#storage.kvGet(key);
kvGet(key: string): string | undefined {
return this.#storage.get(key);
}

kvSet(key: string, value: string): void {
this.#storage.kvSet(key, value);
this.#storage.set(key, value);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/kernel/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export * from './messages/index.js';
export { Kernel } from './Kernel.js';
export type { KernelStore } from './kernel-store.js';
export type { KVStore } from './kernel-store.js';
export { Vat } from './Vat.js';
export { Supervisor } from './Supervisor.js';
export type { StreamEnvelope, StreamEnvelopeReply } from './stream-envelope.js';
Expand Down
Loading