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.

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

kernel.kpResolution increments refcounts, which gets in the way #7213

Open
warner opened this issue Mar 22, 2023 · 0 comments
Open

kernel.kpResolution increments refcounts, which gets in the way #7213

warner opened this issue Mar 22, 2023 · 0 comments
Labels
bug Something isn't working SwingSet package: SwingSet test vaults_triage DO NOT USE

Comments

@warner
Copy link
Member

warner commented Mar 22, 2023

Describe the bug

kernel.kpResolution(kpid) is used by unit tests to ask the kernel for the resolution of a given promise. We use this in tests that do kernel.queueToKref() or controller.queueToVatRoot(), to send a message to a vat and then learn about its results.

In commit 5ace0aa, nearly two years ago (and probably in a fit of desperation), I changed kpResolution() to increment the refcount of all the returned krefs by one. I think I had a test in which the result promise was resolved with new object references, to which I wanted to send more messages (so newobj = kpResolution(kpid); queueToKref(newobj, ..)), and the test was failing because GC was killing off the new objects before my new message could be delivered to them. My reasoning in the commit comment shows that the unit test is acting like a kind of vat, but if it were a real vat, its c-list entries would establish a refcount on the new objects, keeping them alive. So I made the unit test behave slightly more like a real vat, incrementing a refcount.

The problem is that now this incref is getting in the way. @gibson042 was working on unit tests in #7170 (comment) which showed a surprisingly high refcount for an otherwise single-referenced object (2,2 instead of 1,1, and 1,2 instead of 0,1). We concluded that the extra refcount came from the kpResolution additions. The test wants to assert that the object is now retired (at least once #7212 is implemented), but the extra count would inhibit that retirement.

In addition, the kpResolution incref would be completely wrong if the same kref were mentioned in more than one result promise. It's not treating the test code as a vat with either 0 or 1 references: it's just blindly incrementing the count by 1 for each appearance in a resolution.

The Fix

@gibson042 and I figured that kpResolution should not do the incref, and instead, test cases which need it should be doing their own increfs.

We can get there through the following sequence of steps:

  • 1: change kernel.kpResolution(kpid) to take an options bag, with a incref: true default
    • and only do the increment if requested
  • 2: for tests (like in kernel should retire abandoned non-reachable objects #7212) that are affected by this, set incref: false
  • 3: add a controller.incref(kref)
  • 4: then find all tests that use kpResolution (I count 35 instances) and change them to use incref: false and add controller.incref() calls
  • 5: once all calls are modified, change the default to incref: false
  • 6: strip out all the now-redundant kpResolution(kpid, { incref: false }) options
  • 7: maybe stop here, or maybe remove the options bag

We might save ourselves some work and use incref: true instead of adding a controller.incref(), but that doesn't really address the "what happens if the same kref appears in multiple results?" double-incref concern.

@gibson042 is likely to do steps 1 and 2 in the course of fixing #7170.

@warner warner added bug Something isn't working SwingSet package: SwingSet test labels Mar 22, 2023
@warner warner added the vaults_triage DO NOT USE label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SwingSet package: SwingSet test vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

1 participant