Skip to content

Commit

Permalink
fix(swingset): retain more references
Browse files Browse the repository at this point in the history
This increments the refcount in several cases that would otherwise allow an
object to be dropped prematurely:

* deviceKeeper: pin objects that pass through the device. Device c-lists lack
the "reachable" flag that was recently added to vat c-lists. To prevent
devices from losing access to any object that was passed in (temporarily held
only by the device, and no vats or run-queue messages), we need to forcibly
increment the refcount during device c-list kernel-to-vat translation. This
pins those krefs forever.

* `kernel.addExport()` is used to pretend that a vat has exported some
object, so we can get a kref on it and e.g. submit exomessages with
`kernel.queueToKref`. If a real vat had imported this kref, that vat's c-list
would maintain a reference to it, keeping the refcount non-zero and
inhibiting GC. But if it's merely a unit test that remembers the kref, there
is no importing c-list to provide the refcount, and the object is in danger
of being collected before the test can finish interacting with it. So this
commit changes `kernel.addExport()` to increment the refcount by one,
replacing the missing importing vat.

* it also increments the refcount on slots of `kpResolution`-retrieved
promise resolution data, for the same reason.
  • Loading branch information
warner committed Jun 15, 2021
1 parent 462e9fd commit 5ace0aa
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 23 deletions.
9 changes: 8 additions & 1 deletion packages/SwingSet/src/kernel/kernel.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,17 @@ function makeError(message, name = 'Error') {
const VAT_TERMINATION_ERROR = makeError('vat terminated');

/*
* Pretend that a vat just exported an object
* Pretend that a vat just exported an object, and increment the refcount on
* the resulting kref so nothing tries to delete it for being unreferenced.
*/
export function doAddExport(kernelKeeper, fromVatID, vref) {
insistVatID(fromVatID);
assert(parseVatSlot(vref).allocatedByVat);
const vatKeeper = kernelKeeper.provideVatKeeper(fromVatID);
const kref = vatKeeper.mapVatSlotToKernelSlot(vref);
// we lie to incrementRefCount (this is really an export, but we pretend
// it's an import) so it will actually increment the count
kernelKeeper.incrementRefCount(kref, 'doAddExport', { isExport: false });
return kref;
}

Expand Down Expand Up @@ -1008,6 +1012,9 @@ export default function buildKernel(
case 'fulfilled':
case 'rejected':
kernelKeeper.decrementRefCount(kpid, 'external');
for (const kref of p.data.slots) {
kernelKeeper.incrementRefCount(kref, 'external');
}
return p.data;
default:
assert.fail(X`invalid state for ${kpid}: ${p.state}`);
Expand Down
25 changes: 21 additions & 4 deletions packages/SwingSet/src/kernel/state/deviceKeeper.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,20 @@ export function initializeDeviceState(kvStore, deviceID) {
*
* @param {*} kvStore The storage in which the persistent state will be kept
* @param {string} deviceID The device ID string of the device in question
* @param {*} addKernelDeviceNode Kernel function to add a new device node to the
* kernel's mapping tables.
*
* @param { addKernelDeviceNode: (deviceID: string) => string,
* incrementRefCount: (kernelSlot: string,
* tag: string?,
* options: {
* isExport: boolean?,
* onlyRecognizable: boolean?,
* },
* ) => undefined),
* } tools
* @returns {*} an object to hold and access the kernel's state for the given device
*/
export function makeDeviceKeeper(kvStore, deviceID, addKernelDeviceNode) {
export function makeDeviceKeeper(kvStore, deviceID, tools) {
insistDeviceID(deviceID);
const { addKernelDeviceNode, incrementRefCount } = tools;

function setSourceAndOptions(source, options) {
assert.typeof(source, 'object');
Expand Down Expand Up @@ -78,6 +85,7 @@ export function makeDeviceKeeper(kvStore, deviceID, addKernelDeviceNode) {
} else {
assert.fail(X`unknown type ${type}`);
}
// device nodes don't have refcounts: they're immortal
const kernelKey = `${deviceID}.c.${kernelSlot}`;
kvStore.set(kernelKey, devSlot);
kvStore.set(devKey, kernelSlot);
Expand Down Expand Up @@ -119,6 +127,15 @@ export function makeDeviceKeeper(kvStore, deviceID, addKernelDeviceNode) {
} else {
assert.fail(X`unknown type ${type}`);
}
// Use isExport=false, since this is an import. Unlike
// mapKernelSlotToVatSlot, we use onlyRecognizable=false, to increment
// both reachable and recognizable, because we aren't tracking object
// reachability on the device clist (deviceSlots doesn't use WeakRefs
// and won't emit dropImports), so we need the clist to hold a ref to
// the imported object forever.
const opts = { isExport: false, onlyRecognizable: false };
incrementRefCount(kernelSlot, `${deviceID}|dk|clist`, opts);

const devSlot = makeVatSlot(type, false, id);

const devKey = `${deviceID}.c.${devSlot}`;
Expand Down
3 changes: 2 additions & 1 deletion packages/SwingSet/src/kernel/state/kernelKeeper.js
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,8 @@ export default function makeKernelKeeper(kvStore, streamStore, kernelSlog) {
initializeDeviceState(kvStore, deviceID);
}
if (!ephemeral.deviceKeepers.has(deviceID)) {
const dk = makeDeviceKeeper(kvStore, deviceID, addKernelDeviceNode);
const tools = { addKernelDeviceNode, incrementRefCount };
const dk = makeDeviceKeeper(kvStore, deviceID, tools);
ephemeral.deviceKeepers.set(deviceID, dk);
}
return ephemeral.deviceKeepers.get(deviceID);
Expand Down
34 changes: 17 additions & 17 deletions packages/SwingSet/test/test-kernel.js
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ test('three-party', async t => {
id: ap,
state: 'unresolved',
policy: 'ignore',
refCount: 1,
refCount: 2,
decider: vatA,
subscribers: [],
queue: [],
Expand Down Expand Up @@ -732,7 +732,7 @@ test('subscribe to promise', async t => {
id: kp,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vat2,
subscribers: [vat1],
queue: [],
Expand Down Expand Up @@ -783,7 +783,7 @@ test('promise resolveToData', async t => {
id: pForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [vatA],
queue: [],
Expand Down Expand Up @@ -858,7 +858,7 @@ test('promise resolveToPresence', async t => {
id: pForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [vatA],
queue: [],
Expand Down Expand Up @@ -928,7 +928,7 @@ test('promise reject', async t => {
id: pForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [vatA],
queue: [],
Expand Down Expand Up @@ -1061,7 +1061,7 @@ test('non-pipelined promise queueing', async t => {
id: p1ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1070,7 +1070,7 @@ test('non-pipelined promise queueing', async t => {
id: p2ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1079,7 +1079,7 @@ test('non-pipelined promise queueing', async t => {
id: p3ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1097,7 +1097,7 @@ test('non-pipelined promise queueing', async t => {
id: p1ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: vatB,
subscribers: [],
queue: [
Expand All @@ -1112,7 +1112,7 @@ test('non-pipelined promise queueing', async t => {
id: p2ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: undefined,
subscribers: [],
queue: [
Expand All @@ -1127,7 +1127,7 @@ test('non-pipelined promise queueing', async t => {
id: p3ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: undefined,
subscribers: [],
queue: [],
Expand Down Expand Up @@ -1186,7 +1186,7 @@ test('pipelined promise queueing', async t => {
id: p1ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1195,7 +1195,7 @@ test('pipelined promise queueing', async t => {
id: p2ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 3,
refCount: 4,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1204,7 +1204,7 @@ test('pipelined promise queueing', async t => {
id: p3ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: undefined,
subscribers: [],
queue: [],
Expand All @@ -1226,7 +1226,7 @@ test('pipelined promise queueing', async t => {
id: p1ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [],
queue: [],
Expand All @@ -1235,7 +1235,7 @@ test('pipelined promise queueing', async t => {
id: p2ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [],
queue: [],
Expand All @@ -1244,7 +1244,7 @@ test('pipelined promise queueing', async t => {
id: p3ForKernel,
state: 'unresolved',
policy: 'ignore',
refCount: 2,
refCount: 3,
decider: vatB,
subscribers: [],
queue: [],
Expand Down

0 comments on commit 5ace0aa

Please sign in to comment.