Skip to content
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

Jump to bottom

fix(swingset): fix refcounts for messages queued to a promise #3270

Merged
merged 3 commits into from
Jun 9, 2021
Merged

fix(swingset): fix refcounts for messages queued to a promise #3270

merged 3 commits into from
Jun 9, 2021

Commits on Jun 9, 2021

  1. chore(swingset): add failing test for #3264

    @warner
    warner committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    9c2351f View commit details
    Browse the repository at this point in the history

  2. chore(swingset): kernelKeeper.incstat() takes delta 

    This just adds an optional `delta=1` parameter so incstat can be used more
efficiently when incrementing multiple times.
    @warner
    warner committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    d7bad9a View commit details
    Browse the repository at this point in the history

  3. fix(swingset): fix refcounts for messages queued to a promise 

    As described in #3264, we were not maintaining correct reference counts when
messages get queued to an unresolved promise.

In the new approach, the lifetime of a message is:

* all krefs (target, args, and optional result) of a message are increfed
  during `doSend`, as before
  * this adds a `type: 'send'` event to the run-queue
* when the `send` event is pulled off the run-queue, all krefs are
  decrefed (as before)
* if the event is delivered to a vat, great
* if the event is delivered to a promise,
  `kernelKeeper.addMessageToPromiseQueue` increfs all krefs
  * (this is new: previously `addMessageToPromiseQueue` did not change the
    refcounts)
* later, if/when the promise is resolved, the messages are transferred
  laterally from the promise queue to `send` events on the run-queue, without
  changing their refcounts
  * (this is new: previously the transfer was done with `doSend`, which
    incremented the refcounts)
  * the counts are decremented when the `send` event is removed from the
    run-queue, as usual

The result is the same number of increments and decrements as before, but the
increment is done earlier, so messages on a promise queue will maintain a
refcount on all their krefs (target, args, and any result). This protects
objects and promises which would otherwise have been eligible for collection
while they sat on the promise queue.

Strictly speaking we don't need to maintain a refcount on the target (which
is also kind of redundant: everything on the queue for promise `kp1`
obviously has `target: 'kp1'`), since that will always be additionally held
by the c-list of the decider (at least). But by making all promise queues do
the same thing as the main run-queue, we can laterally transfer messages from
promise- to run-queue without DB churn (decrementing then immediately
incrementing the counts).

This change moves the responsibility for the lateral transfer from
`notifySubscribersAndQueue` in kernel.js to
`kernelKeeper.resolveKernelPromise()`, deleting the former entirely and
merging the subscription loop into `doResolve`.

closes #3264
    @warner
    warner committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    0da6eea View commit details
    Browse the repository at this point in the history