Skip to content

don’t expose stale values #333

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 24, 2022
Merged

don’t expose stale values #333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff10e2b
don’t expose invalidated values
mbostock May 21, 2022
ba33c34
throw variable_stale
mbostock May 23, 2022
f0dd56c
try-catch
mbostock May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ function config(output) {
"SQLiteDatabaseClient",
"Workbook",
"ZipArchive",
"ZipArchiveEntry"
"ZipArchiveEntry",
"Runtime",
"RuntimeError",
"Variable",
"Module",
"Library",
"Inspector",
]
}
})
Expand Down
16 changes: 14 additions & 2 deletions src/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,20 @@ async function module_value(name) {
v._observer = true;
this._runtime._dirty.add(v);
}
await this._runtime._compute();
return v._promise;
return module_revalue(this._runtime, v);
}

// If the variable is redefined before its value resolves, try again.
async function module_revalue(runtime, variable) {
await runtime._compute();
const version = variable._version;
return variable._promise.then((value) => {
if (variable._version !== version) return module_revalue(runtime, variable);
return value;
}, (error) => {
if (variable._version !== version) return module_revalue(runtime, variable);
throw error;
});
}

function module_derive(injects, injectModule) {
Expand Down
1 change: 1 addition & 0 deletions src/variable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ function variable_defineImpl(name, inputs, definition) {
this._name = name;
}

++this._version;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed when you call variable.define (or a wrapper like variable.import) so that the version is incremented synchronously. If we don’t do this, then the version won’t be implemented until the runtime recomputes on the next animation frame, which means we could leak a value that we already know is invalid because the variable was redefined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I’ve realized that this isn’t sufficient: it’s not enough to just increment the version on the redefined variable; we would also need to increment it on any downstream variable (any variable that depends on this variable). I didn’t want to do that because it involves traversing the dataflow graph, dealing with circular dependencies, etc. But I think we will need to add that here or else it’s still possible to report invalidated values. It shouldn’t have much performance cost because, other than load, variables are rarely redefined, and we should be able to skip the traversal if the variable has never been evaluated.

runtime._updates.add(this);
runtime._compute();
return this;
Expand Down