[SR-786] addObserverForName causes a memory leak on pure swift objects

[swift-ci]

Previous ID	SR-786
Radar	None
Original Reporter	Ciurus (JIRA User)
Type	Bug

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug
Assignee	None
Priority	Medium

md5: e5d782af342b2ec76296a7751f32a0e3

Issue Description:

NSNotificationCenter.defaultCenter().addObserverForName("X", object: nil, queue: NSOperationQueue.mainQueue()) { [weak self] notification in
            self?.method()
}

If "self" is a pure swift object (not NSObject) it'll cause the closure/block to retain self in spite of it being declared as weak. It caused a memory leak in my classes that were pure swift and worked propery in my NSObject classes.

[belkadan]

What version of Swift are you using? (If in Xcode, which Xcode? If it's a beta, which beta?)

@jckarter, sound familiar?

[jckarter]

Pure Swift weak references lazily reclaim their memory since we never implemented the side-table implementation for them. The memory won't be reclaimed until the closure is invoked. Is that what you're seeing? Note that the reference cycle should still be severed.

[swift-ci]

Comment by Michal (JIRA)

Sorry, forgot to mention: Xcode 7.2.1

Yes, that's exactly what I'm seeing.

[jckarter]

It shouldn't be a true leak, then, just delayed memory reclamation. We had intended to move to a more ObjC-like eager reclamation model, but that requires a slower implementation.

[swift-ci]

Comment by Michal (JIRA)

by "delayed", you mean delayed until the closure is invoked ? A good practice is to delete observers as fast as you can (willDisappear in ViewControllers for example), so most of the times the closure will not be invoked.

[jckarter]

Delayed until the closure is either invoked or destroyed. Releasing all of the weak references to the object should also reclaim its memory, unless that's where we have a bug.

[swift-ci]

Comment by Michal (JIRA)

Ok, that's the problem I have. I'm removing the observer in `deinit` - and naturally deinit is not being called, so the closure is not destroyed.

[jckarter]

OK, that's a real problem then. deinit is supposed to be called when the last strong reference goes away, even if weak references keep the memory around.

[swift-ci]

Comment by Michal (JIRA)

Im testing that now. I only assumed that.

[swift-ci]

Comment by Michal (JIRA)

Ok, deinit is being called. Sorry for the confusion.

The problem here is that my deinit looks like that

    deinit {
        NSNotificationCenter.defaultCenter().removeObserver(self)
    }

And that's not how you remove observers that are added with "addObserverForName:usingBlock", so the closure doesn't get destroyed after all.

The remaining question is: is the fact that the closure keeps the object alive in spite of [weak self] a bug, or not ?
Is this ticket still valid, or not ?

Also it's important to remember that for iOS9 it is not mandatory anymore to remove observers as it will not cause an exception to be raised.

[jckarter]

There's a discussion about this on swift-dev:

https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151207/000253.html

We had planned to eventually move to a more ObjC-like implementation, but @mikeash made some arguments for keeping the current implementation model (with fixes for some concurrency problems he discovered). Problems like this might necessitate us sticking to the original plan.

[swift-ci]

Comment by Michal (JIRA)

Ok, please change the state of this ticket to whatever you see fit 🙂)

Thanks!

[Dante-Broggi]

Is this resolved, or no longer valid? If either, this should be closed.