Swiftlint: unneeded_notification_center_removal considered harmful

[mildm8nnered]

I just recently upgraded to 0.40.2, and came across unneeded_notification_center_removal

This is documented (https://realm.github.io/SwiftLint/unneeded_notification_center_removal.html) as follows:

"Observers are automatically unregistered on dealloc (iOS 9 / macOS 10.11) so you should’t call removeObserver(self) in the deinit."

But that is not actually true, although it is a common misconception.

Apple says:

"If your app targets iOS 9.0 and later or macOS 10.11 and later, you do not need to unregister an observer that you created with this function. If you forget or are unable to remove an observer, the system cleans up the next time it would have posted to it."

https://developer.apple.com/documentation/foundation/notificationcenter/1415360-addobserver

"Do not need" here, means, in the sense of "do not need if you do not mind your app taking up more and more memory".

The Notification Center keeps all registered notifications in a big table. When an observer is deallocated, the entry in this table is not removed. As per Apple, if the notification for that object fires after it's been deallocated, then it will be automatically removed from the table.

But for most, or at least many, observers this never happens, because this would often have been crashing behaviour before iOS 9/macOS 10.11, and often the posters and observers of notifications share a common lifecycle, so to post a notification to a deallocated object would have been a programming error anyway.

So, because automatic deregistration will never happen in many cases, the net effect of this rule is that the notification center's registration table will fill up with stale entries over time, consuming memory.

In apps that rely extensively on Notifications, the memory and performance cost can become material.

This behaviour can be trivially demonstrated by something like adding something like

extension NSObject {
    @objc func fire() {
        print("fired for \(self)")
    }
}

    @IBAction func registerForNotifications(_ sender: AnyObject?) {
        let object = NSObject()
        for _ in 0..<100_000 {
            NotificationCenter.default.addObserver(object, selector: #selector(fire), name: NSNotification.Name(rawValue: "test"), object: nil)
        }
    }

to a sample project. If you hook up registerForNotifications to a button, you can watch the apps memory consumption grow. If you print NotificationCenter.default in the debugger, you'll be able to see all the stale entries.

The documentation for this rule could definitely be improved.

I don't think it should be enabled by default, as it is actively harmful

Ideally, this rule wouldn't exist at all, and but we would have the converse missing_notification_center_removal instead.

[natalia-osa]

I've also found the following points about removing block based observers. I would like to hear your thoughts about these since both of them aren't so clear in my opinion.

---

1. From Apple documentation on block based observers:
   >You must invoke removeObserver(_:) or removeObserver(_:name:object:) before the system deallocates any object that addObserver(forName:object:queue:using:) specifies.

---

1. From https://useyourloaf.com/blog/unregistering-nsnotificationcenter-observers-in-ios-9/
   >Note that this does not apply if you are using a block based observer:
   >
   >Block based observers via the -[NSNotificationCenter addObserverForName:object:queue:usingBlock] method still need to be un-registered when no longer in use since the system still holds a strong reference to these observers.

[mildm8nnered]

@natalia-osa - iI don't use block-based observers usually, so I hadn't noticed that, but yep, as I read the documentation, it looks like calling NotificationCenter.removeObserver(self) will remove all block based notification observations by an object, and that Apple recommends explicit removal in these cases.

If I'm reading that right, unneeded_notification_center_removal is even more harmful in those cases, unless it's somehow detecting that block-based KVO is in use, and not firing then. The documentation at https://realm.github.io/SwiftLint/unneeded_notification_center_removal.html does not suggest that's the case.

[jpsim]

Thanks for raising this issue @mildm8nnered, I'm removing the rule in #3407.