misc cleanup #9661
misc cleanup #9661
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
3c531f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://85b02f87.agoric-sdk.pages.dev |
| Branch Preview URL: | https://ta-zoe-misc.agoric-sdk.pages.dev |
| bundleCache = await makeNodeBundleCache(dest, options, loadModule, pid); | ||
| providedCaches.set(uniqueDest, bundleCache); |
There was a problem hiding this comment.
This looks like more than a cleanup, but rather, a genuine bug fix with potentially different (now correct) behavior. Looking at makeNodeBundleCache, I see it is declared async and so certainly returns a promise. The old code stored this promise into the providedCaches collection, which is almost certainly wrong. As the price of correcting this, the new code also waits a turn before storing it into the cache. Is this a possible race condition?
Afterwards, both old and new code returns a promise for the bundleCache, though the old code mistypes it. As long as the types pass, I'm not concerned about the bug fix to the corrected type. But what about the bug fix to what is stored in the collection?
I am also more than happy to see this bug corrected. I only raise this because the PR description sounds like this PR has no such significant behavioral change.
So, no change suggested to the code. But perhaps the PR description should discuss this possible behavioral change?
There was a problem hiding this comment.
Thanks for the careful review. I didn't consider the race condition. I suppose that's why it was storing promises. I've reverted that part of the change and just fixed the types.
| // in heapVowE or also reflects a runtime deficiency. But this | ||
| // case it not used yet anyway. We disable it | ||
| // with at-ts-ignore rather than at-ts-expect-error because | ||
| // the dependency-graph tests complains that the latter is unused. |
There was a problem hiding this comment.
Answering a question from the PR description:
I think it is reasonable to make the dependency-graph tests required.
| @@ -28,8 +28,6 @@ const { ownKeys } = Reflect; | |||
| * @returns {(ir: InstanceRecord) => InstanceState} | |||
| */ | |||
| export const makeInstanceRecordStorage = baggage => { | |||
| provide(baggage, 'instanceRecord', () => undefined); | |||
There was a problem hiding this comment.
Any idea why this was there? Likewise with the other pure deletions of unneeded baggage stuff?
There was a problem hiding this comment.
Just leftover. Explained in the commit: 19407b8
|
I did a "rerun failed jobs" to clear a likely flake. We'll see what happens. |
Added in https://github.com/Agoric/agoric-sdk/blame/2c28414a0ab2dd4c05bb9f6955b8d658b484253d/packages/zoe/src/instanceRecordStorage.js#L31 9f4d39b made it obsolete but neglected to remove
This reverts commit 7f4d952. Co-authored-by: Turadg Aleahmad <turadg@agoric.com>
incidental
Description
Some cleanups noticed in the course of #9659
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
"Dependency Graph" test has been failing on the recent change to
replay-membrane.js. Maybe that should be a Required test in the repo.Upgrade Considerations
This changes Zoe but in a fully backwards and forward compatible way. I think it can get into master and reach chain whenever Zoe is next upgraded.