Skip to content

Commit

Permalink
fix(swingset): comms: remove deleteToRemoteMapping
Browse files Browse the repository at this point in the history
When the comms vat resolves a promise, it begins a two-part retirement
process. Right after it sends the `resolve` message to the remote, it deletes
the outbound half of the c-list, because this promise ID is partially
retired, and the comms vat won't be referencing it in any outbound messages
again.

The inbound half is retained until the other side acknowledges the resolution
message (i.e. some inbound messages arrives which demonstrates awareness of
the `notify`, by including an `ackSeqNum` at least as large as the `seqNum`
of the outbound `notify`).

This changes the code to retain both sides of the c-list entry until the ack
is received. I think this winds up being slightly cleaner, and most
importantly it retains our ability to map local-ref to remote-ref, which
enables an upcoming change to `deleteRemoteMapping` / `deleteKernelMapping`
to only take a single argument (lref, not lref+rret), which is a lot cleaner,
and avoids some error cases.

refs #3306
  • Loading branch information
warner committed Jun 20, 2021
1 parent 0abb12d commit 6d15240
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 16 deletions.
1 change: 0 additions & 1 deletion packages/SwingSet/src/vats/comms/clist-inbound.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export function makeInbound(state) {
insistRemoteType('promise', rpid);
const remote = state.getRemote(remoteID);
const lpid = remote.mapFromRemote(flipRemoteSlot(rpid));
remote.deleteToRemoteMapping(lpid);
remote.enqueueRetirement(rpid);
cdebug(`comms begin retiring ${remoteID} ${rpid} ${lpid}`);
}
Expand Down
5 changes: 0 additions & 5 deletions packages/SwingSet/src/vats/comms/remote.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ export function makeRemote(state, store, remoteID) {
state.decrementRefCount(lref, `{rref}|${remoteID}|clist`);
}

function deleteToRemoteMapping(lref) {
store.delete(`${remoteID}.c.${lref}`);
}

function nextSendSeqNum() {
return Number(store.getRequired(`${remoteID}.sendSeq`));
}
Expand Down Expand Up @@ -150,7 +146,6 @@ export function makeRemote(state, store, remoteID) {
mapToRemote,
addRemoteMapping,
deleteRemoteMapping,
deleteToRemoteMapping,
allocateRemoteObject,
skipRemoteObjectID,
allocateRemotePromise,
Expand Down
22 changes: 12 additions & 10 deletions packages/SwingSet/test/test-comms.js
Original file line number Diff line number Diff line change
Expand Up @@ -426,21 +426,21 @@ test('retire result promise on inbound message', t => {
_('k>r', [pResult, false, 42]);
_('a<r', [paResult, false, 42]);

// Now the kernel c-list entries and the outbound remote c-list entry should
// be gone but the inbound remote c-list entry should still be there. The
// result promise should also still be there, but be in a resolved state.
// Now the kernel c-list entries should be gone but the inbound/outbound
// remote c-list entries should still be there. The result promise should
// also still be there, but be in a resolved state.
t.falsy(state.mapFromKernel(refOf(pResult)));
t.falsy(state.mapToKernel(plResult));
t.falsy(remoteA.mapToRemote(plResult));
t.truthy(remoteA.mapToRemote(plResult));
t.truthy(remoteA.mapFromRemote(refOf(paResult)));
t.is(state.getPromiseStatus(plResult), 'fulfilled');

// Then the remote sends some traffic, which will contain an ack
_('a>m', oaLarry, 'more', undefined);
_('k<m', oLarry, 'more', undefined);

// And now the inbound remote c-list entry and the promise itself should be
// gone too.
// And now the remote c-list entries and the promise itself should be gone
t.falsy(remoteA.mapToRemote(plResult));
t.falsy(remoteA.mapFromRemote(refOf(paResult)));
t.is(state.getPromiseStatus(plResult), undefined);

Expand Down Expand Up @@ -555,24 +555,26 @@ test('retire outbound promises', t => {
_('k>r', [pPromise1, false, [pPromise2]], [pPromise2, false, [pPromise1]]);
_('a<r', [paPromise1, false, [paPromise2]], [paPromise2, false, [paPromise1]]);

// Now the kernel c-list entries and the outbound remote c-list entries should
// be gone but the inbound remote c-list entries should still be there
// Now the kernel c-list entries should be gone, but the inbound/outgoung
// remote c-list entries should still be there
t.falsy(state.mapFromKernel(refOf(pPromise1)));
t.falsy(state.mapToKernel(plPromise1));
t.falsy(remoteA.mapToRemote(plPromise1));
t.truthy(remoteA.mapToRemote(plPromise1));
t.truthy(remoteA.mapFromRemote(refOf(paPromise1)));

t.falsy(state.mapFromKernel(refOf(pPromise2)));
t.falsy(state.mapToKernel(plPromise2));
t.falsy(remoteA.mapToRemote(plPromise2));
t.truthy(remoteA.mapToRemote(plPromise2));
t.truthy(remoteA.mapFromRemote(refOf(paPromise2)));

// Then the remote sends some traffic, which will contain an ack
_('a>m', oaLarry, 'more', undefined);
_('k<m', oLarry, 'more', undefined);

// And now the inbound remote c-list entries should be gone too.
t.falsy(remoteA.mapToRemote(plPromise1));
t.falsy(remoteA.mapFromRemote(refOf(paPromise1)));
t.falsy(remoteA.mapToRemote(plPromise2));
t.falsy(remoteA.mapFromRemote(refOf(paPromise2)));

done();
Expand Down

0 comments on commit 6d15240

Please sign in to comment.