Fix KVO bugs related to cyclical observer dependencies (#608)#612
Merged
Conversation
- Replaced fragile dependent-keypath visited tracking with node keys `(object pointer, key name)` in dependency expansion. - Added explicit ancestor/on-stack tracking to distinguish true cycles from safe revisits - On revisit: - if node is on ancestor stack: treat as cycle and stop expansion - if visited but not on stack: allow expansion to continue - This prevents infinite recursive rebuilds on cyclic dependencies while preserving correct branch wiring after nil-to-non-nil rematerialization and shared-path alias cases.
Contributor
Author
|
@rmf the tests fail for gcc. I suppose they are using the old KVO implementation. Should I just skip them for gcc? |
Contributor
|
Tests are great, so we prefer not to disable them if possible. eg. That will run the tests, but report them as dashed hopes rather than failures, so the testsuite as a whole passes but reminds us there is work to be done on the portability. |
Contributor
Author
Member
|
Yes, I tested it on Android with our app. 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR solves two KVO bugs described in #608.
I used the suggested solution from the original issue, while keying the nodes using the object pointer and the key name.
Additionally, I added a bunch of tests here. These are essentially fuzzing attempts by an LLM and have been very useful counter examples for why previous approaches I tried locally didn't work. I compared the results of running each of these against Apple's objc implementation and the behavior now matches Apple.
Let me know if the tests should be kept and whether I should add a change log entry.