Reimplement invaliding Notification attributes cache
#19395
Conversation
The original implementation overrides `NSManagedObject.willChangeValue` which must not be overridden as mentioned in the API doc. See https://developer.apple.com/documentation/coredata/nsmanagedobject/1506229-willchangevalue
| // | ||
| guard notification.managedObjectContext?.concurrencyType == .mainQueueConcurrencyType else { | ||
| return | ||
| } |
There was a problem hiding this comment.
The original observer logic is kept, including this one here. But I think not invaliding the cache just because the notification object is in a background context is problematic, see the unit test below for more.
You can test the changes in WordPress from this Pull Request by:
|
You can test the changes in Jetpack from this Pull Request by:
|
mokagio
left a comment
mokagio
left a comment
There was a problem hiding this comment.
Love how you wrote a test to show the threading issue!
I started looking at the git blame, then got interrupted, then side tracked, and I'm only now coming back to my WIP-review.
I didn't get to the bottom of how that .mainQueueConcurrencyType came about, but the comment states it was a matter of optimization. Maybe it was a case of premature optimization?
I'd be okay with removing the restriction and see how the app behaves.
| /// | ||
| fileprivate var cachedHeaderAndBodyContentGroups: [FormattableContentGroup]? | ||
|
|
||
| private let cachedAttributesObserver: NotificationCachedAttributesObserver? = nil |
There was a problem hiding this comment.
Is the explicit = nil assignment intentional?
I think the compiler is already doing that for us.
There was a problem hiding this comment.
Not for let variable apparently. But this property is changed to a var and I've removed the nil assignment.
There was a problem hiding this comment.
Not for
letvariable apparently
Blimey! I didn't notice it was a let 😳
| cachedAttributesObserver = NotificationCachedAttributesObserver() | ||
| for attr in Notification.cachedAttributes { | ||
| addObserver(cachedAttributesObserver!, forKeyPath: attr, context: nil) | ||
| } |
There was a problem hiding this comment.
What do you think of doing the following, to remove the force unwrap?
| cachedAttributesObserver = NotificationCachedAttributesObserver() | |
| for attr in Notification.cachedAttributes { | |
| addObserver(cachedAttributesObserver!, forKeyPath: attr, context: nil) | |
| } | |
| let cachedAttributesObserver = NotificationCachedAttributesObserver() | |
| for attr in Notification.cachedAttributes { | |
| addObserver(cachedAttributesObserver, forKeyPath: attr, context: nil) | |
| } | |
| self.cachedAttributesObserver = cachedAttributesObserver |
The force unwrap is of course okay in the way it's used now, but who's to tell how the code will evolve? There might end up being a code path in which it's nil in the future.
Maybe I'm overthinking it 😅
There was a problem hiding this comment.
👍 addressed in da753bd. It's always good to not use force unwrap.
This will reduce amount of KVO observer calls, now that only the fetched instances have KVO observer registered, not the inserted ones.
The original implementation overrides
NSManagedObject.willChangeValuewhich must not be overridden as mentioned in the API doc.Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
None.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txtif necessary.