Skip to content

Commit

Permalink
Fix: call stoppedObserving for _all_ non-self-owned observers
Browse files Browse the repository at this point in the history
  • Loading branch information
pcantrell committed Oct 3, 2016
1 parent 474292b commit 0cd076f
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 6 deletions.
11 changes: 5 additions & 6 deletions Source/Siesta/Resource/ResourceObserver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,16 @@ public protocol ResourceObserver
Called when this observer stops observing a resource, if the observer itself still exists.
Use for making `removeObservers(ownedBy:)` trigger other cleanup.
- Warning: Only observers that are **retained outside of Siesta** will receive this message.
This method is **not** called if deallocation of the observer itself is what caused it to stop observing.
- Warning: This method is **not** called for self-owned observers when the observer itself being deallocated is
what caused it to stop observing. This is because there is no way for Siesta to know that observer is _about_
to be deallocated; it can only check whether the observer is already gone.
For example:
var myObserver = MyObserver()
resource.addObserver(myObserver) // myObserver is self-owned, so...
myObserver = nil // this deallocates it, but...
// ...myObserver never receives stoppedObserving(resource:), because
// there is no way for Siesta to know that it should stop observing
// other than checking whether it is already deallocated.
// ...myObserver never receives stoppedObserving(resource:).
In the situation above, `MyObserver` should implement any end-of-lifcycle cleanup using `deinit`.
*/
Expand Down Expand Up @@ -388,7 +387,7 @@ internal class ObserverEntry: CustomStringConvertible
// Look for weak refs which refer to objects that are now gone
externalOwners.filterInPlace { $0.value != nil }

observerRef.strong = !externalOwners.isEmpty
observerRef.strong = !observerIsOwner || !externalOwners.isEmpty
}

var isDefunct: Bool
Expand Down
26 changes: 26 additions & 0 deletions Tests/Functional/ResourceObserversSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,32 @@ class ResourceObserversSpec: ResourceSpecBase
awaitObserverCleanup(for: resource())
}

it("receives removal notification if not externally retained but not self-owned")
{
var observer2: TestObserverWithExpectations? = TestObserverWithExpectations()
observer2?.expect(.observerAdded)
resource().addObserver(observer2!, owner: observer) // not self-owned

observer.expectStoppedObserving()
observer2!.expectStoppedObserving()
observer2 = nil
resource().removeObservers(ownedBy: observer)
awaitObserverCleanup(for: resource())
}

it("does not receive removal notification if self-owned and not externally retained")
{
var observer2: TestObserverWithExpectations? = TestObserverWithExpectations()
observer2?.expect(.observerAdded)
resource().addObserver(observer2!) // self-owned

observer.expectStoppedObserving()
// No expectStoppedObserving() for observer2!
observer2 = nil
resource().removeObservers(ownedBy: observer)
awaitObserverCleanup(for: resource())
}

it("receives a notification every time it is removed and re-added")
{
let observer2 = TestObserverWithExpectations()
Expand Down

0 comments on commit 0cd076f

Please sign in to comment.