Skip to content
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

Cycles detected later than expected while polling producers for changes #66

Open
shaylew opened this issue Mar 29, 2024 · 0 comments
Open
Labels
bug Something isn't working Semantic details

Comments

@shaylew
Copy link
Collaborator

shaylew commented Mar 29, 2024

The polyfill doesn't blackhole a Computed until producerRecomputeValue is called. But evaluation of other computations can happen before that, while polling producers for changes. This can lead to a situation where:

  • In a previous run, we created Computeds A and B with A depending on B.
  • We're currently making sure A is up to date, after making a change that will cause B to need to rerun. We get to a state that looks like:
    • A'sproducerUpdateValueVersion
    • A's consumerPollProducersForChange
    • B's producerUpdateValueVersion
    • B's producerRecomputeValue
  • At this point, if B's compute function calls A.get(), we don't see this as a cycle. Instead we go through this process again, polling A's producers, discovering that B needs to rerun, and trying to recompute B... which finally throws.

In essence, producerUpdateValueVersion produces a stack with two types of relevant frames in it:

  • consumerPollProducersForChange, when we don't know whether a node is dirty and have to poll its producers;
  • producerRecomputeValue, when we do know we have to recompute.

But only producerRecomputeValue is blackholing the Computed. This seems too late to me; by the time we ask for A while A is already doing producerUpdateValueVersion we know something has gone wrong. B's reevaluation should throw as soon as it reads A, IMO. As far as I can tell the current state isn't lax enough to construct a true cycle, but the fact that we don't catch the cycle until the second time through is likely to be observable in debugging tools/error messages.

I have a test case in 406f7ac to use for debugging, but the assertion isn't ideal -- we don't have enough information on the exception to easily tell that it threw too late/for the wrong reasons. Technically A should run twice -- the second time it'll just get the (already-computed) cached exception when it reads B. (It's enough to get to vitest --inspect-brk --no-file-parallelism and see what's on the stack, though, so I thought it was worth sharing.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Semantic details
Projects
None yet
Development

No branches or pull requests

2 participants