Skip to content

Commit

Permalink
fix: tweaks from PR review
Browse files Browse the repository at this point in the history
  • Loading branch information
@FUDCo
FUDCo committed Aug 26, 2020
1 parent 7fa2661 commit 3c51b0f
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 31 deletions.
8 changes: 3 additions & 5 deletions packages/SwingSet/src/kernel/id.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,10 @@ export function insistVatID(s) {
if (s !== `${s}`) {
throw new Error(`not a string`);
}
if (s !== 'none') {
if (!s.startsWith(`v`)) {
throw new Error(`does not start with 'v'`);
}
Nat(Number(s.slice(1)));
if (!s.startsWith(`v`)) {
throw new Error(`does not start with 'v'`);
}
Nat(Number(s.slice(1)));
} catch (e) {
throw new Error(`'${s} is not a 'vNN'-style VatID: ${e.message}`);
}
Expand Down
32 changes: 19 additions & 13 deletions packages/SwingSet/src/kernel/kernel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ function makeError(s) {
}

const VAT_TERMINATION_ERROR = makeError('vat terminated');
const UNKNOWN_VAT_ERROR = makeError('unknown vat');

export default function buildKernel(kernelEndowments, kernelOptions = {}) {
const {
Expand Down Expand Up @@ -312,8 +311,8 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
const vat = ephemeral.vats.get(vatID);
kernelKeeper.incStat('dispatches');
kernelKeeper.incStat('dispatchDeliver');
if (!vat || vat.dead || vatID === 'none') {
resolveToError(msg.result, UNKNOWN_VAT_ERROR);
if (!vat) {
resolveToError(msg.result, VAT_TERMINATION_ERROR);
} else {
const kd = harden(['message', target, msg]);
const vd = vat.translators.kernelDeliveryToVatDelivery(kd);
Expand All @@ -326,8 +325,11 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
const { type } = parseKernelSlot(target);
if (type === 'object') {
const vatID = kernelKeeper.ownerOfKernelObject(target);
vatID === 'none' || insistVatID(vatID);
await deliverToVat(vatID, target, msg);
if (vatID) {
await deliverToVat(vatID, target, msg);
} else {
resolveToError(msg.result, VAT_TERMINATION_ERROR);
}
} else if (type === 'promise') {
const kp = kernelKeeper.getKernelPromise(target);
if (kp.state === 'fulfilledToPresence') {
Expand All @@ -351,11 +353,15 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
kernelKeeper.addMessageToPromiseQueue(target, msg);
} else {
insistVatID(kp.decider);
const vat = ephemeral.vats.get(kp.decider);
if (vat.enablePipelining) {
await deliverToVat(kp.decider, target, msg);
const deciderVat = ephemeral.vats.get(kp.decider);
if (deciderVat) {
if (deciderVat.enablePipelining) {
await deliverToVat(kp.decider, target, msg);
} else {
kernelKeeper.addMessageToPromiseQueue(target, msg);
}
} else {
kernelKeeper.addMessageToPromiseQueue(target, msg);
resolveToError(msg.result, VAT_TERMINATION_ERROR);
}
}
} else {
Expand Down Expand Up @@ -385,7 +391,7 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
insistKernelType('promise', kpid);
const vat = ephemeral.vats.get(vatID);
kernelKeeper.incStat('dispatches');
if (!vat || vat.dead) {
if (!vat) {
kdebug(`dropping notify of ${kpid} to ${vatID} because vat is dead`);
} else {
const p = kernelKeeper.getKernelPromise(kpid);
Expand Down Expand Up @@ -628,7 +634,7 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
// the VatManager+VatWorker will see the error case, but liveslots will
// not
function vatSyscallHandler(vatSyscallObject) {
if (ephemeral.vats.get(vatID).dead) {
if (!ephemeral.vats.get(vatID)) {
// This is a safety check -- this case should never happen unless the
// vatManager is somehow confused.
console.error(`vatSyscallHandler invoked on dead vat ${vatID}`);
Expand Down Expand Up @@ -712,7 +718,7 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {

function removeVatManager(vatID) {
const old = ephemeral.vats.get(vatID);
ephemeral.vats.set(vatID, harden({ dead: true }));
ephemeral.vats.delete(vatID);
old.notifyTermination(null);
return old.manager.shutdown();
}
Expand Down Expand Up @@ -903,7 +909,7 @@ export default function buildKernel(kernelEndowments, kernelOptions = {}) {
for (const vatID of ephemeral.vats.keys()) {
logStartup(`Replaying transcript of vatID ${vatID}`);
const vat = ephemeral.vats.get(vatID);
if (vat.dead) {
if (!vat) {
logStartup(`skipping reload of dead vat ${vatID}`);
} else {
// eslint-disable-next-line no-await-in-loop
Expand Down
42 changes: 32 additions & 10 deletions packages/SwingSet/src/kernel/state/kernelKeeper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const enableKernelPromiseGC = true;
// v$NN.c.$vatSlot = $kernelSlot = ko$NN/kp$NN/kd$NN
// v$NN.t.$NN = JSON(transcript entry)
// v$NN.t.nextID = $NN
// v$NN.dead = missing | true

// d$NN.o.nextID = $NN
// d$NN.c.$kernelSlot = $deviceSlot = o-$NN/d+$NN/d-$NN
Expand Down Expand Up @@ -251,8 +250,10 @@ export default function makeKernelKeeper(storage) {

function ownerOfKernelObject(kernelSlot) {
insistKernelType('object', kernelSlot);
const owner = getRequired(`${kernelSlot}.owner`);
insistVatID(owner);
const owner = storage.get(`${kernelSlot}.owner`);
if (owner) {
insistVatID(owner);
}
return owner;
}

Expand Down Expand Up @@ -439,17 +440,37 @@ export default function makeKernelKeeper(storage) {
const kpPrefix = `${vatID}.c.p`;
const kernelPromisesToReject = [];
for (const k of storage.getKeys(`${vatID}.`, `${vatID}/`)) {
// The store semantics ensure this iteration is lexicographic. Any
// changes to the creation of the list of promises need to preserve this
// in order to preserve determinism.
// The current store semantics ensure this iteration is lexicographic.
// Any changes to the creation of the list of promises to be rejected (and
// thus to the order in which they *get* rejected) need to preserve this
// ordering in order to preserve determinism. TODO: we would like to
// shift to a different deterministic ordering scheme that is less fragile
// in the face of potential changes in the nature of the database being
// used.
if (k.startsWith(koPrefix)) {
// The void for an object exported by a vat will always be of the form
// `o+NN`. The '+' means that the vat exported the object (rather than
// importing it) and therefor the object is owned by (i.e., within) the
// vat. The corresponding void->koid c-list entry will thus always
// begin with `vMM.c.o+`. In addition to deleting the c-list entry, we
// must also delete the corresponding kernel owner entry for the object,
// since the object will no longer be accessible.
const koid = storage.get(k);
const ownerKey = `${koid}.owner`;
const ownerVat = storage.get(ownerKey);
if (ownerVat === vatID) {
storage.set(ownerKey, 'none');
storage.delete(ownerKey);
}
} else if (k.startsWith(kpPrefix)) {
// The vpid for a promise imported or exported by a vat (and thus
// potentially a promise for which the vat *might* be the decider) will
// always be of the form `p+NN` or `p-NN`. The corresponding vpid->kpid
// c-list entry will thus always begin with `vMM.c.p`. Decider-ship is
// independent of whether the promise was imported or exported, so we
// have to look up the corresponding kernel promise table entry to see
// whether the vat is the decider or not. If it is, we add the promise
// to the list of promises that must be rejected because the dead vat
// will never be able to act upon them.
const kpid = storage.get(k);
const p = getKernelPromise(kpid);
if (p.state === 'unresolved' && p.decider === vatID) {
Expand All @@ -459,9 +480,10 @@ export default function makeKernelKeeper(storage) {
storage.delete(k);
}
// TODO: deleting entries from the dynamic vat IDs list requires a linear
// scan; this collection ought to be represented in a way that makes it
// efficient to remove an entry from it, though this should be OK as long as
// we keep the list short.
// scan of the list; arguably this collection ought to be represented in a
// different way that makes it efficient to remove an entry from it, though
// for the time being the linear list should be OK enough as long as we keep
// the list short.
const DYNAMIC_IDS_KEY = 'vat.dynamicIDs';
const oldDynamicVatIDs = JSON.parse(getRequired(DYNAMIC_IDS_KEY));
const newDynamicVatIDs = oldDynamicVatIDs.filter(v => v !== vatID);
Expand Down
6 changes: 3 additions & 3 deletions packages/SwingSet/test/vat-admin/terminate/test-terminate.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ test('terminate', async t => {
'GOT QUERY 3',
'foreverP.catch vat terminated',
'query3P.catch vat terminated',
'foo4P.catch unknown vat',
'foo4P.catch vat terminated',
'afterForeverP.catch vat terminated',
'done',
]);
Expand Down Expand Up @@ -86,7 +86,7 @@ test('dispatches to the dead do not harm kernel', async t => {
t.deepEqual(c2.dump().log, [
'b: p1b = I so resolve',
'b: p2b fails vat terminated',
'm: live 2 failed: unknown vat',
'm: live 2 failed: vat terminated',
]);
}
});
Expand Down Expand Up @@ -139,6 +139,6 @@ test('dead vat state removed', async t => {
controller.queueToVatExport('bootstrap', 'o+0', 'phase2', capargs([]));
await controller.run();
t.is(storage.get('vat.dynamicIDs'), '[]');
t.is(storage.get('ko26.owner'), 'none');
t.is(storage.get('ko26.owner'), undefined);
t.is(Array.from(storage.getKeys('v6.', 'v6/')).length, 0);
});

0 comments on commit 3c51b0f

Please sign in to comment.