Skip to content

Commit

Permalink
refactor(swingset): create vat state non-lazily
Browse files Browse the repository at this point in the history
Previously, vat state initialization (e.g. setting counters to zero)
happened lazily, the first time that `provideVatKeeper()` was
called. When creating a new vat, the pattern was:

  vk = kernelKeeper.provideVatKeeper();
  vk.setSourceAndOptions(source, options);

Now, we initialize both counters and source/options explicitly, in
`recordVatOptions`, when the static/dynamic vat is first defined:

  kernelKeeper.createVatState(vatID, source, options);

In the future, this will make it easier for `provideVatKeeper` to rely
upon the presence of all vat state keys, especially `options`.

Previously, vatKeeper.getOptions() would tolerate a missing key by
returning empty options. The one place where this was
needed (terminating a barely-created vat because startVat() failed,
using getOptions().critical) was changed, so this tolerance is no
longer needed, and was removed. The tolerance caused bug #9157 (kernel
doesn't panic when it should), which continues to be present, but at
least won't cause a crash.

refs #8980
  • Loading branch information
warner committed Jun 13, 2024
1 parent 97fa24a commit fa05927
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 27 deletions.
11 changes: 9 additions & 2 deletions packages/SwingSet/src/kernel/kernel.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,7 @@ export default function buildKernel(
* @param {SwingSetCapData} info
*/
async function terminateVat(vatID, shouldReject, info) {
const vatKeeper = kernelKeeper.provideVatKeeper(vatID);
const critical = vatKeeper.getOptions().critical;
let critical = false;
insistCapData(info);
// ISSUE: terminate stuff in its own crank like creation?
// TODO: if a static vat terminates, panic the kernel?
Expand All @@ -261,6 +260,14 @@ export default function buildKernel(
// check will report 'false'. That's fine, there's no state to
// clean up.
if (kernelKeeper.vatIsAlive(vatID)) {
// If there was no vat state, we can't make a vatKeeper to ask for
// options. For now, pretend the vat was non-critical. This will fail
// to panic the kernel for startVat failures in critical vats
// (#9157). The fix will add .critical to CrankResults, populated by a
// getOptions query in deliveryCrankResults() or copied from
// dynamicOptions in processCreateVat.
critical = kernelKeeper.provideVatKeeper(vatID).getOptions().critical;

// Reject all promises decided by the vat, making sure to capture the list
// of kpids before that data is deleted.
const deadPromises = [...kernelKeeper.enumeratePromisesByDecider(vatID)];
Expand Down
9 changes: 6 additions & 3 deletions packages/SwingSet/src/kernel/state/kernelKeeper.js
Original file line number Diff line number Diff line change
Expand Up @@ -1288,15 +1288,17 @@ export default function makeKernelKeeper(kernelStorage, kernelSlog) {
maybeFreeKrefs.clear();
}

function createVatState(vatID, source, options) {
initializeVatState(kvStore, transcriptStore, vatID, source, options);
}

function provideVatKeeper(vatID) {
insistVatID(vatID);
const found = ephemeral.vatKeepers.get(vatID);
if (found !== undefined) {
return found;
}
if (!kvStore.has(`${vatID}.o.nextID`)) {
initializeVatState(kvStore, transcriptStore, vatID);
}
assert(kvStore.has(`${vatID}.o.nextID`), `${vatID} was not initialized`);
const vk = makeVatKeeper(
kvStore,
transcriptStore,
Expand Down Expand Up @@ -1610,6 +1612,7 @@ export default function makeKernelKeeper(kernelStorage, kernelSlog) {
getVatIDForName,
allocateVatIDForNameIfNeeded,
allocateUnusedVatID,
createVatState,
provideVatKeeper,
vatIsAlive,
evictVatKeeper,
Expand Down
30 changes: 21 additions & 9 deletions packages/SwingSet/src/kernel/state/vatKeeper.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,30 @@ const FIRST_DEVICE_ID = 70n;
* @param {*} transcriptStore Accompanying transcript store
* @param {string} vatID The vat ID string of the vat in question
* TODO: consider making this part of makeVatKeeper
* @param {SourceOfBundle} source
* @param {RecordedVatOptions} options
*/
export function initializeVatState(kvStore, transcriptStore, vatID) {
export function initializeVatState(
kvStore,
transcriptStore,
vatID,
source,
options,
) {
assert(options.workerOptions, `vat ${vatID} options missing workerOptions`);
assert(source);
assert('bundle' in source || 'bundleName' in source || 'bundleID' in source);
assert.typeof(options, 'object');
const count = options.reapInterval;
assert(count === 'never' || isNat(count), `bad reapCountdown ${count}`);

kvStore.set(`${vatID}.o.nextID`, `${FIRST_OBJECT_ID}`);
kvStore.set(`${vatID}.p.nextID`, `${FIRST_PROMISE_ID}`);
kvStore.set(`${vatID}.d.nextID`, `${FIRST_DEVICE_ID}`);
kvStore.set(`${vatID}.source`, JSON.stringify(source));
kvStore.set(`${vatID}.options`, JSON.stringify(options));
kvStore.set(`${vatID}.reapInterval`, `${count}`);
kvStore.set(`${vatID}.reapCountdown`, `${count}`);
transcriptStore.initTranscript(vatID);
}

Expand Down Expand Up @@ -125,16 +144,10 @@ export function makeVatKeeper(

function getOptions() {
/** @type { RecordedVatOptions } */
const options = JSON.parse(kvStore.get(`${vatID}.options`) || '{}');
const options = JSON.parse(getRequired(`${vatID}.options`));
return harden(options);
}

function initializeReapCountdown(count) {
count === 'never' || isNat(count) || Fail`bad reapCountdown ${count}`;
kvStore.set(`${vatID}.reapInterval`, `${count}`);
kvStore.set(`${vatID}.reapCountdown`, `${count}`);
}

function updateReapInterval(reapInterval) {
reapInterval === 'never' ||
isNat(reapInterval) ||
Expand Down Expand Up @@ -656,7 +669,6 @@ export function makeVatKeeper(
setSourceAndOptions,
getSourceAndOptions,
getOptions,
initializeReapCountdown,
countdownToReap,
updateReapInterval,
nextDeliveryNum,
Expand Down
4 changes: 1 addition & 3 deletions packages/SwingSet/src/lib/recordVatOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ export const makeVatOptionRecorder = (kernelKeeper, bundleHandler) => {
critical,
meterID,
});
const vatKeeper = kernelKeeper.provideVatKeeper(vatID);
vatKeeper.setSourceAndOptions(source, vatOptions);
vatKeeper.initializeReapCountdown(vatOptions.reapInterval);
kernelKeeper.createVatState(vatID, source, vatOptions);
};

/**
Expand Down
8 changes: 8 additions & 0 deletions packages/SwingSet/test/clist.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ test(`clist reachability`, async t => {
const s = kk.kvStore;
kk.createStartingKernelState({ defaultManagerType: 'local' });
const vatID = kk.allocateUnusedVatID();
const source = { bundleID: 'foo' };
const options = { workerOptions: 'foo', reapInterval: 1 };
kk.createVatState(vatID, source, options);
const vk = kk.provideVatKeeper(vatID);

const ko1 = kk.addKernelObject('v1', 1);
Expand Down Expand Up @@ -98,12 +101,17 @@ test('getImporters', async t => {

kk.createStartingKernelState({ defaultManagerType: 'local' });
const vatID1 = kk.allocateUnusedVatID();
const source = { bundleID: 'foo' };
const options = { workerOptions: 'foo', reapInterval: 1 };
kk.createVatState(vatID1, source, options);
kk.addDynamicVatID(vatID1);
const vk1 = kk.provideVatKeeper(vatID1);
const vatID2 = kk.allocateUnusedVatID();
kk.createVatState(vatID2, source, options);
kk.addDynamicVatID(vatID2);
const vk2 = kk.provideVatKeeper(vatID2);
const vatID3 = kk.allocateUnusedVatID();
kk.createVatState(vatID3, source, options);
kk.addDynamicVatID(vatID3);
const vk3 = kk.provideVatKeeper(vatID3);

Expand Down
20 changes: 10 additions & 10 deletions packages/SwingSet/test/state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,8 +511,11 @@ test('vatKeeper', async t => {
const store = buildKeeperStorageInMemory();
const k = makeKernelKeeper(store, null);
k.createStartingKernelState({ defaultManagerType: 'local' });

const v1 = k.allocateVatIDForNameIfNeeded('name1');
const source = { bundleID: 'foo' };
const options = { workerOptions: 'foo', reapInterval: 1 };
k.createVatState(v1, source, options);

const vk = k.provideVatKeeper(v1);
// TODO: confirm that this level of caching is part of the API
t.is(vk, k.provideVatKeeper(v1));
Expand Down Expand Up @@ -547,18 +550,15 @@ test('vatKeeper.getOptions', async t => {
const store = buildKeeperStorageInMemory();
const k = makeKernelKeeper(store, null);
k.createStartingKernelState({ defaultManagerType: 'local' });

const v1 = k.allocateVatIDForNameIfNeeded('name1');
const vk = k.provideVatKeeper(v1);
const bundleID =
'b1-00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000';
vk.setSourceAndOptions(
{ bundleID },
{
workerOptions: { type: 'local' },
name: 'fred',
},
);
const source = { bundleID };
const workerOptions = { type: 'local' };
const options = { workerOptions, name: 'fred', reapInterval: 1 };
k.createVatState(v1, source, options);

const vk = k.provideVatKeeper(v1);
const { name } = vk.getOptions();
t.is(name, 'fred');
});
Expand Down

0 comments on commit fa05927

Please sign in to comment.