-
Notifications
You must be signed in to change notification settings - Fork 212
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.
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
orphaned objects are not fully GCed: one refcount key is left in the DB #3378
Comments
There were two bugs in the kernel's garbage-collection handling of orphaned objects (exports of a vat which is later terminated). * Sending a message to an orphaned vat caused the object's refcount to be incremented, but never decremented again, preventing it from being collected. The root cause was `kernelKeeper.kernelObjectExists` using `.owner` to decide whether the object still exists. Orphaned objects have a `.refcount` but no `.owner`. The fix is to just test `.refcount` instead of `.owner`. #3376 * `kernelKeeper.processRefcounts()` wants to check the `isReachable` flag of the exporting vat, to know whether they need a dropExport message, but orphaned objects have no exporting vat anymore. The check crashed the kernel when it attempted to get a vatKeeper for `undefined`. The immediate fix is to skip this check for orphaned objects, which avoids the crash (#3377). But we still need a fix to delete the refcount=0,0 entry (#3378). closes #3376 closes #3377 refs #3378
There were two bugs in the kernel's garbage-collection handling of orphaned objects (exports of a vat which is later terminated). * Sending a message to an orphaned vat caused the object's refcount to be incremented, but never decremented again, preventing it from being collected. The root cause was `kernelKeeper.kernelObjectExists` using `.owner` to decide whether the object still exists. Orphaned objects have a `.refcount` but no `.owner`. The fix is to just test `.refcount` instead of `.owner`. #3376 * `kernelKeeper.processRefcounts()` wants to check the `isReachable` flag of the exporting vat, to know whether they need a dropExport message, but orphaned objects have no exporting vat anymore. The check crashed the kernel when it attempted to get a vatKeeper for `undefined`. The immediate fix is to skip this check for orphaned objects, which avoids the crash (#3377). But we still need a fix to delete the refcount=0,0 entry (#3378). closes #3376 closes #3377 refs #3378
I'm leaning towards the latter fix (have |
Might be associated with (or fixed at the same time as) #3439 |
I was working on agoric-sdk/packages/SwingSet/test/test-gc-kernel.js Lines 1168 to 1170 in bef63e5
which is commented out because of this bug. I uncommented that part, and it passes. Looking at the code more closely, I think it's been fixed: agoric-sdk/packages/SwingSet/src/kernel/state/kernelKeeper.js Lines 1276 to 1280 in bef63e5
That code is run when I'll make a PR that uncomments the test clause. |
Describe the bug
#3377 was about a kernel crash that occurred when an orphaned object was processed by
processRefcounts()
. The fix avoids the crash, but fails to clean up after the object fully. It leaves the${kref}.refCount
DB key in place. Normally that would be deleted when aretireExport
GC action was sent to the exporter, but of course orphaned objects have no exporter. By not pushing aretireExport
GC action for the late owner vat, we also fail to delete the.refCount
key.I'm not sure of the cleanest solution yet. Maybe have a special kind of "no target vat"
retireExport
GC action, which deletes the key but doesn't tell any vat about it? MaybeprocessRefcounts()
should notice 1:recognizable=0
and 2:ownerVatID === undefined
and delete the key itself, instead of pushing a GC action (but would the DB key change appear in the right commit window?).The consequence is that we leave one DB key (
${kref}.refCount = 0,0
) around for each orphaned export of any dead vat. We don't kill vats very frequently yet (well, ok, not intentionally), so this is low-priority.The text was updated successfully, but these errors were encountered: