jonasman / MulticastDelegate

An elegant multicast delegate written in swift
MIT License
149 stars 28 forks source link

remove delegate on deinit #20

Closed liufannnn closed 5 years ago

liufannnn commented 5 years ago
class ViewController: UIViewController, DemoServiceDelegate {

    @IBOutlet weak var topLabel: UILabel!
    @IBOutlet weak var bottomLabel: UILabel!

    let dataService = DemoService.defaultService

    override func viewDidLoad() {
        super.viewDidLoad()

        dataService.delegate += self
    }

    @IBAction func getDataTapped(_ sender: AnyObject) {
        dataService.getData("TestingMulticast")
    }

    //MARK: DemoServiceDelegate
    func gotYourData(_ value:String) {
        topLabel.text = value
    }

    deinit {
        dataService.delegate -= self
    }
}

We need to remove delegate manually, In this example.

RodolfoAntonici commented 5 years ago

You don't need to, the delegates are kept in a NSHashTable, elements kept there are weak, so they are deallocated when the object ceases to exists, so you don't need to manually remove the self reference on the deinit

jonasman commented 5 years ago

You don't need to, the delegates are kept in a NSHashTable, elements kept there are weak, so they are deallocated when the object ceases to exists, so you don't need to manually remove the self reference on the deinit

exactly

liufannnn commented 5 years ago

open class MulticastDelegate<T> {

    /// The delegates hash table.
    private let delegates: NSHashTable<AnyObject>

    /**
     *  Use the property to check if no delegates are contained there.
     *
     *  - returns: `true` if there are no delegates at all, `false` if there is at least one.
     */
    public var isEmpty: Bool {

        return delegates.count == 0
    }

    var count: Int {
        return delegates.count
    }
   ...
}
class ViewController: UIViewController, DemoServiceDelegate {

    @IBOutlet weak var topLabel: UILabel!
    @IBOutlet weak var bottomLabel: UILabel!

    let dataService = DemoService.defaultService

    override func viewDidLoad() {
        super.viewDidLoad()

        dataService.delegate += self
    }

    @IBAction func getDataTapped(_ sender: AnyObject) {
        dataService.getData("TestingMulticast")
        present(TssViewController(), animated: true, completion: nil)
    }

    //MARK: DemoServiceDelegate
    func gotYourData(_ value:String) {
        topLabel.text = value
    }

    override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {
        print(dataService.delegate.count)
    }

}
class TssViewController: UIViewController, DemoServiceDelegate {

    override func viewDidLoad() {
        super.viewDidLoad()

        // Do any additional setup after loading the view.
        view.backgroundColor = .red
        DemoService.defaultService.delegate += self
    }

    override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {
        print(DemoService.defaultService.delegate.count)
        dismiss(animated: true, completion: nil)
    }

    //MARK: DemoServiceDelegate
    func gotYourData(_ value:String) {

    }
}

ViewController.present(TssViewController(), animated: true, completion: nil) and dismiss(animated: true, completion: nil) The dataService.delegate.count will not change.

jonasman commented 5 years ago

In theory the HashTable will handle it by itself. you can check if your TssViewController instance is still in memory after deinit or not. Or try to call your delegates and validate that the app does not crash.

The HashTable (if you did not change the config) holds a weak reference to your TssViewController so as soon as the instance is deallocated the reference is gone. The counter probably does not update righ away as an optimization on the HashTable, but here im not sure.

RodolfoAntonici commented 5 years ago

I've exposed the delegates property on the MulticastDelegate.swift from a file private to regular

So on both ViewControllers touches began I've added the following line:

print(dataService.delegate.delegates.allObjects)
print(dataService.delegate.delegates.count)

This will print out the array of objects contained in the NSHashTable, my outputs: Before presenting the TssViewController

[<NSHashTableTest.ViewController: 0x7feb2240ad70>]
1

After presenting it:

[<NSHashTableTest.ViewController: 0x7feb2240ad70>, <NSHashTableTest.TssViewController: 0x7feb2251a530>]
2

After dismissing it:

[<NSHashTableTest.ViewController: 0x7feb2240ad70>]
2

The objects are right but the count are messed up, what happens is that the NSHashTable will keep counting nil references, but they won't be invoked because they doesn't exist anymore, to fix the isEmpty property I did this modification:

public var isEmpty: Bool {
        return delegates.allObjects.count == 0
}

@jonasman so yeah, we have an issue, is nothing that will break things, but if anyone is checking the isEmpty before doing the calls, maybe they are calling nothing. @KevinCoderX But chill, there's no memory leak ;)

jonasman commented 5 years ago

@KevinCoderX #21 should fix your count issue I will later create a new pod version