Skip to content

Commit

Permalink
fix(swingset): gcAndFinalize needs two post-GC setImmediates on V8
Browse files Browse the repository at this point in the history
When testing the upcoming #3482 fix, we observed that a vat method which
simply delegates to a second vat, like this:

```js
  getInvitationTarget: () => E(zoe).getInvitationZoe(),
```

would not always drop the "invitation" Presence returned by both methods,
when run under Node.js. It dropped the Presence correctly perhaps 20% of the
time, and the other 80% it failed to drop it. Under XS it dropped it all of
the time.

Node.js started working correctly all of the time (N=8 or so) when we changed
`gcAndFinalize` to do *two* `setImmediate`s after the `gc()`, instead of just
one.

I'd like to add a unit test that fails with a similar probability, but I
haven't been able to come up with one. Either they fail to collect the object
all of the time, or none of the time.

refs #3240

Hopefully it fixes that, but I won't be sure until I run more load-generator
tests and look for growth in the object counts over time. And I'd like to add
that test before closing the issue.
  • Loading branch information
warner committed Jul 22, 2021
1 parent 0ebfb3a commit 1fee5ef
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 0 deletions.
2 changes: 2 additions & 0 deletions packages/SwingSet/src/gc-and-finalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,7 @@ export function makeGcAndFinalize(gcPower) {
gcPower();
// this gives finalizers a chance to run
await new Promise(setImmediate);
// Node.js seems to need another for promises to get cleared out
await new Promise(setImmediate);
};
}
4 changes: 4 additions & 0 deletions packages/SwingSet/test/test-gc-and-finalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,7 @@ provokeGC(globalThis.gc).then(data => issueCommand(ArrayBuffer.fromString(JSON.s
t.is(wrState, 'weakref is dead');
t.is(finalizerState, 'finalizer was called');
});

// TODO: exercise 'return E(zoe).foo()' like in test-gc-vat from #3482, and
// demonstrate that Node.js requires 2 setImmediates after the gc() to allow
// everything to get collected. Then update #3240.

0 comments on commit 1fee5ef

Please sign in to comment.