Add tests for cancellation in fixpoints#1085
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
| let t2 = thread::spawn({ | ||
| let mut db_writer = db_writer; | ||
| move || { | ||
| db_writer.trigger_lru_eviction(); |
There was a problem hiding this comment.
I guess we have the same problem when someone calls trigger_cancellation, since it doesn't bump a new revision either.
There was a problem hiding this comment.
Yea, both of those have the same issue. We don't really need to merge this right now, just put this up so I don't lose it. So yea, unsure what we wanna do here, either we need to remove these functions (which would be a shame, their point is that they do not bump the revision), or we need to figure out how to handle poisoning differently. In r-a we'll avoid calling these for now as the gain from their use is marginal.
|
What makes cycle recovery tricky is that there's no guarantee that queries will be executed in the same order after they've been cancelled. E.g., if we have A possibility is to walk the entire dependency tree and nuke all memos. |
|
You mean the plan changes the next time someone enters the cycle? What would be the issue with it changing? |
Yes, because the user calls a different root query. The issue with it is that it requires invalidating all provisional memoized values that participated in the query because the iteration count isn't sufficient to distinguish a provisional memoized value that was computed when entering from |
|
I believe we're now also seeing instances of this in ty (astral-sh/ty#3339 (comment)) If I remember correctly, why we saw a hang previously was that a query that now should re-execute never did because But maybe this is no longer an issue, now that we rewrote cycle handling. |
|
I added the tests as part of #1100 |
revision_bump_clears_poisoned_memoreplicates the issue discussed in #1063