Skip to content

pipeline state bugs #88

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

Closed
dherman opened this issue Oct 8, 2015 · 3 comments
Closed

pipeline state bugs #88

dherman opened this issue Oct 8, 2015 · 3 comments
Assignees

Comments

@dherman
Copy link

dherman commented Oct 8, 2015

A few bugs in the management of internal state in the pipeline:

  • We don't want to save any intermediate results of the pipeline once we move on to later states. So the conditional in the beginning of each Request<<X>> operation should check if it's in any later pipeline stage, not just link-or-later.
  • Instead of an error promise, if the data is gone it should succeed with a tombstone value of undefined. This indicates that the stage is complete but we no longer have the data available. One way of looking at this is the .load method enables prefetching and signals completion, and that completion provides the intermediate value only if it's still available. Prefetching logic needs to be prepared to handle this case. (Note that the high-level .import method has none of this subtlety.) Another way of looking at it is if you want to be guaranteed to get the data as it's available, you need to be setting the hooks, not using .load.
  • In order to avoid race conditions, each Request<<X>> stage should have a three-armed conditional:
    • If we're already in exactly the stage X, we have the promise for its result already, so just return that.
    • If we're in a later stage (> X), return a tombstone promise.
    • If we're in an earlier stage, call the preceding Request<<X-1>> stage and .then its result. Note that this guarantees we aren't in a later stage than X-1 so we are guaranteed not to get a tombstone.
  • The .provide method should be async -- it should Promise.resolve its argument and then update the state. (The result of .provide should be a Promise<undefined> instead of undefined, so that the caller can be notified when the update has happened.) This resolves several issues:
    • It ensures that the user argument gets promise-assimilated (i.e. in case it's thenable).
    • It pushes race conditions to between event turns.
    • It provides a guarantee that the only synchronous question you should be able to ask of the pipeline state -- namely, what pipeline stage are we in -- should be stable throughout the current event turn. (Iteration is also synchronously sensitive to the set of keys in the registry, but the values it provides should be promises.)
@caridy caridy self-assigned this Oct 8, 2015
@dherman
Copy link
Author

dherman commented Oct 8, 2015

Note that the last bullet point will resolve the TODO question in CommitInstantiated: we have an invariant (should be an assert towards the beginning of the abstract operation's steps) that the value is fully assimilated. It can never be a thenable.

caridy added a commit to caridy/loader that referenced this issue Oct 8, 2015
caridy added a commit that referenced this issue Oct 9, 2015
caridy added a commit that referenced this issue Oct 12, 2015
related to issue #88: refactor to use a list of stages instead of internal slots for each state.
@caridy
Copy link
Contributor

caridy commented Oct 23, 2015

related to #96

@caridy
Copy link
Contributor

caridy commented Dec 1, 2015

solved by PR #97

@caridy caridy closed this as completed Dec 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants