Correct oversights & editing errors in virtual object code #1967
Conversation
| if (initialize) { | ||
| delete initialRepresentative.initialize; | ||
| initialize(...args); | ||
| } | ||
| delete initialData[initializationInProgress]; |
There was a problem hiding this comment.
Should initializationInProgress be a WeakSet rather than a symbol? As a symbol on the object, it can be manipulated by the initialize function.
There was a problem hiding this comment.
I suppose a WeakSet would work equally well, but since the symbol is known only to the virtual object manager and the property is non enumerable I don't think there's a way for the initialize function to manipulate it or even know about it. Is there some JavaScript-fu that I'm ignorant of that would enable that?
There was a problem hiding this comment.
(Actually, that's an academic question: since the WeakSet version has less mechanism I'll go with that anyway.)
There was a problem hiding this comment.
property is non enumerable I don't think there's a way for the initialize function to manipulate it or even know about it
IMPORTANT SAFETY TIP
Reflect.getOwnKeys and Object.getOwnPropertyDescriptors, among other things, reveals the names of all own properties on the object, whether they are strings or symbols, whether they are so-called enumerable, or not. Anything you use as a property name on an object is discoverable by anything with access to that object.
The JavaScript terminology "enumerable" is only of concern to a narrow range of issues: for-in loops, Object.{keys, values, entries}, Object.assign, Object spread and Object rest {...x}, and JSON. Probably a handful of other things not on the top of my head. It does not mean these properties cannot be enumerated!
There was a problem hiding this comment.
RIght, don't cross the streams.
FUDCo
force-pushed
the
further-virtual-objects-stuff
branch
from
aa8f1e7 to
480d0a0
Compare
FUDCo
force-pushed
the
further-virtual-objects-stuff
branch
from
480d0a0 to
2eb5f71
Compare
| } | ||
|
|
||
| function reanimate(vobjID) { | ||
| return makeRepresentative(cache.lookup(vobjID), false); | ||
| return makeRepresentative(cache.lookup(vobjID), false).self; |
There was a problem hiding this comment.
Since it now makes an instance kit, how about?
| return makeRepresentative(cache.lookup(vobjID), false).self; | |
| return makeInstanceKit(cache.lookup(vobjID), false).self; |
| @@ -26,9 +26,9 @@ export function makeCache(size, fetch, store) { | |||
| const cache = { | |||
| makeRoom() { | |||
| while (liveTable.size > size && lruTail) { | |||
| if (lruTail.rawData[initializationInProgress]) { | |||
| if (initializationsInProgress.has(lruTail.rawData)) { | |||
There was a problem hiding this comment.
I never understood what race condition this protects against. I'm reviewing ignoring that question.
There was a problem hiding this comment.
I can explain it in detail if you care, but tl;dr: async calls from init can potentially get you into a situation where you fill the entire cache with objects being initialized and it tries to page out an object whose initialization is still in progress and thus has no valid state object to swap out.
|
closes #1960 |
|
closes #1973 |
|
closed #1846 |
This corrects some small but significant errors in the previous pass on this code: