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

Dependency tracking (abusing the waitFor API) #144

Closed
politician opened this Issue Feb 2, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@politician

politician commented Feb 2, 2015

It looks like there is some code to prevent nested dispatch and some cycle-detection logic in https://github.com/facebook/flux/blob/master/src/Dispatcher.js. However, it looks like that defense can be subverted by combining waitFor with named function expressions.

dispatcher.register(function handler () {
    var tok = dispatcher.register(handler); // appends a new handler to _callbacks, increments id
    dispatcher.waitFor([tok]); // directly invokes the new handler
    // without a base case, this doesn't terminate (stack overflow)
});

dispatcher.dispatch({}); 

This contrived example reveals a couple of problems. First, _callbacks is not locked for updates during the scope of a dispatch operation. Second, the way dependencies are expressed using this API is open to confusion and error. A quick fix might be to assert invariant(!_isDispatching) within register and unregister, but consider redesigning the register function to accept an array of tokens as a parameter. Then you could resolve a proper dependency graph. See RequireJS for an example of this sort of API.

@fisherwebdev

This comment has been minimized.

Show comment
Hide comment
@fisherwebdev

fisherwebdev Feb 4, 2015

Contributor

Interesting point about the potential for abuse, but I disagree that we want to pass an array of tokens to register(). This would create the same dependency graph for every action, and we want different dependency graphs per action.

I like the idea of the invariants in register and unregister, though. I will float that idea internally at FB to @yungsters and others, and if the idea is accepted by those folks, I'll ask you for a pull request.

Contributor

fisherwebdev commented Feb 4, 2015

Interesting point about the potential for abuse, but I disagree that we want to pass an array of tokens to register(). This would create the same dependency graph for every action, and we want different dependency graphs per action.

I like the idea of the invariants in register and unregister, though. I will float that idea internally at FB to @yungsters and others, and if the idea is accepted by those folks, I'll ask you for a pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 5, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

ghost commented Aug 5, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@kyldvs

This comment has been minimized.

Show comment
Hide comment
@kyldvs

kyldvs Nov 18, 2015

Contributor

Opened #296 to track adding in these invariants.

Contributor

kyldvs commented Nov 18, 2015

Opened #296 to track adding in these invariants.

@kyldvs kyldvs closed this Nov 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment