Skip to content

Conversation

@mbostock
Copy link
Member

If a variable is redefined before its value resolves, we shouldn’t expose it via module.value. Instead, we should retry to get the variable’s new value.

(It’s possible that this could continue forever if the variable is continuously redefined—e.g., it is both slow asynchronous and downstream from a fast generator—but this is consistent with what you’d in a notebook and hence I feel correct.)

(It’s already the case that observers do not report these invalidated values when fulfilling or rejecting.)

@mbostock mbostock requested a review from duaneatat May 21, 2022 14:48
src/variable.js Outdated
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.

@mbostock mbostock requested a review from duaneatat May 23, 2022 15:08
@mbostock
Copy link
Member Author

mbostock commented May 23, 2022

Can you take another look please @duaneatat? I’ve addressed the issue I raised in ba33c34 and I’m feeling good about this now!

@mbostock mbostock changed the title don’t expose invalidated values don’t expose stale values May 23, 2022
@mbostock mbostock merged commit 6862e8e into main May 24, 2022
@mbostock mbostock deleted the mbostock/fix-redefined-value branch May 24, 2022 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants