Skip to content
This repository has been archived by the owner on Aug 29, 2021. It is now read-only.

Normative: [[AsyncEvaluating]] field instead of evaluating-async state #114

Merged
merged 3 commits into from
Jun 21, 2019

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented Jun 9, 2019

This PR converts the "evaluating-async" module state into a separate [[AsyncEvaluating]] boolean field.

We have had a bunch of PRs recently all dealing with the same root problem in cycles, and that is that the "evaluating-async" state is tracking two separate constraints:

  1. Is this execution a currently pending async execution?
  2. Ensure we maintain a strongly connected transition from "evaluating" to "evaluating-async" in cycles.

The issue is that during cycle execution, these two information constraints cannot both be maintained due to the fact that we need to transition states in a cycle as one strongly connected component. So we are missing the state in the field model, that represents "is this module in a cycle, and currently asynchronously evaluating, but pending fulfillment".

My initial incorrect attempt to fix this at #90 as well as the need to introduce the cycle boolean and even the case in #109 are all different manifestations of this same gap in the data model.

Running the case of #109 (comment) against this version of the algorithm, we can see that this new state then properly handles the conversion of cyclic to acyclic completion graphs.

This PR is based on top of the PR at #99 which should probably land first.

@guybedford
Copy link
Collaborator Author

guybedford commented Jun 9, 2019

I've also included the fix / invariant of #115 in this PR as well, implementing the (2) as discussed there.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@guybedford
Copy link
Collaborator Author

@Ms2ger would you feel comfortable approving this one as well while you have the context? Perhaps by independently confirming it resolves #109?

Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Let's say this is okay. Please do squash and land #99 first, and then squash and land this.

@guybedford
Copy link
Collaborator Author

I've gone ahead and rebased this to the latest master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants