Force cache eviction when instance classes don't match.#540
Force cache eviction when instance classes don't match.#540freakboy3742 wants to merge 1 commit into
Conversation
samschott
left a comment
samschott
left a comment
There was a problem hiding this comment.
Can the reverse also happen? We already have the type that we like in the cache (e.g., ObjCDictInstance) and evict it in favour of one that we don't like (e.g., ObjCInstance)? Or is this prevented by all "type preferences" being correctly registered eventually and never being unregistered?
| # (like ObjCMutableDictionary) has been used - an ObjCInstance | ||
| # will respond to the same selectors as ObjCMutableDictionary, | ||
| # but it won't have the helper methods (like __setitem__) that | ||
| # make the wrapper useful. |
There was a problem hiding this comment.
I find this explanation bit hard to understand, especially the 1st sentence "when the instance class doesn't match". Are we talking about Python or ObjC classes here? I gather from the remaining text and the implementation that you mean Python classes.
I'll leave a suggested rewrite, but feel free to reject or adapt. I understand that 'easy to understand' is very subjective :)
We also evict the cached instance when its Python type does not
match what we want to use to represent the given object in Python.
For example, we want to represent a NSDictionary instance as
ObjCDictInstance to add Python dict API methods (like __setitem__).
If we find a ObjCInstance instead in the cache, we evict it.
There was a problem hiding this comment.
Agreed that's a better explanation of this particular implementation; what I'm trying to work out (in response to @mhsmith's queries) is whether there is a better way to frame the original cache hit based on the actual ObjC class data. It feels like there should be... but I've been going mad trying to work out what that formulation would be. All the obvious stuff seems to have weird edge case failures.
|
Never mind that comment, I'm just catching up on #539. You believe the issue is caused by the memory address being reused by a different object. |
|
#543 is likely a better solution for this problem. |
|
Closing on the basis that #543 is a much more comprehensive (and less hacky) solution to the problem. |
3 participants
If a cache hit is found for a object pointer, check the underlying ObjC class name and the class instance. If either are a mismatch, report a cache miss. This ensures that collection types always have their Python wrappers applied, even if they are originally created (and cached) as a base ObjCInstance.
I haven't added a test for this because I haven't been able to find a way to reproduce the issue programmatically - it depends upon cache eviction behaviour, and the only place I've seen this problem occur is via beeware/toga-chart#191.
Fixes #539.
Fixes beeware/toga-chart#191.
PR Checklist: