Memory leak from HubConnection and HubConnectionConnectionDelegate

[kostasfilios]

Due to Swift is a retain counting memory management language,
recommended the usage of "weak" reference at delegate due to strong reference
will leak on a retain cycle and a zobby object.
In this case we have a memory leak between HubConnection and HubConnectionConnectionDelegate, the way to reproduce it is easy.
In ViewController add a foreach ( 5 times ) method in OKAction ( row 37-48 ), start SignalR server and run the application,
after a couple of seconds you will notice in memory grapgh that there are many instance of HubConnection where should exist only one due to variable chatHubConnection. This "zobby" HubConnection exists due to retain cycle between HubConnection and HubConnectionConnectionDelegate.

[moozzyk]

Before you dig the rabbit hole deeper, can you:

• explain exactly where the retain cycle occurs
• provide code to reproduce the issue and steps to verify the leak.

As it stands now, the issue mentions a leak between HubConnection and HubConnectionConnectionDelegate, but the change is trying to change many other references to weak references, seeing what sticks. Plus, a lot of included changes seem unrelated.

[kostasfilios]

Thank you Pawel for your fast replay.

1. The retain cycle
   This issues occures in many points as you can see from the snapshots (below), the issue exists due to
   a) Keeps a retain lock on delegates between classes
   b) Uses 'self' in closures, this self passes as strong reference which means tha the closure ( which also passes as reference ) captures the 'self' of a class,which means that the reference count of class counts up without letting it being removed from memory due to retain count never will be zero. You can read about it here

2. How to reproduce.

You can replace the code of viewDidAppear with this one ( the only difference is that this one starts 10 connections but with out keeping them in any variable only the last one in chatHubConnection )

override func viewDidAppear(_ animated: Bool) {
        let alert = UIAlertController(title: "Enter your Name", message:"", preferredStyle: UIAlertController.Style.alert)
        alert.addTextField() { textField in textField.placeholder = "Name"}
        let OKAction = UIAlertAction(title: "OK", style: .default) { action in
            self.name = alert.textFields?.first?.text ?? "John Doe"
            (1...10).forEach { item in
                let randomTimeAwait = Double.random(in: 0.5..<0.7) * Double(Int.random(in: 1..<7))
                DispatchQueue.global().asyncAfter(deadline: .now() + randomTimeAwait) { [weak self] in
                    guard let self else { return }
                    self.chatHubConnectionDelegate = ChatHubConnectionDelegate(controller: self)
                    let connection = HubConnectionBuilder(url: URL(string: self.serverUrl)!)
                        .withLogging(minLogLevel: .debug)
                        .withAutoReconnect()
                        .withHubConnectionDelegate(delegate: self.chatHubConnectionDelegate!)
                        .build()
                    self.chatHubConnection = connection
                    self.chatHubConnection!.on(method: "NewMessage", callback: {[weak self] (user: String, message: String) in
                        guard let self else { return }
                        self.appendMessage(message: "\(user): \(message)")
                    })
                    self.chatHubConnection!.start()
                    DispatchQueue.global().asyncAfter(deadline: .now() + 3) {
                        connection.stop()
                    }
                }
            }
        }
        alert.addAction(OKAction)
        self.present(alert, animated: true)
    }

At this example in XCode memory graph you can find out many instances without any reference in viewcontroller param chatHubConnection aka zombie objects

As you can see many of them ( leaks ) are in dispatch_queue or swift closure context due to "self" is capturing inside the callback.

P.S. If you agree i can move in a deeper investigation on closures and delegate, this PR is a fast finding fix not a deep search on earch row of your code.
Also keep in mind that leaking HubConnection with HubConnectionConnectionDelegate can be due to leak of HubConnection with 'self' passed in closure, but from lib perpective the application that uses it can't find that HubConnection is the reason

[moozzyk]

Thanks a lot for the details! I will take a deeper look soon.

[kostasfilios]

Thanks !! Feel free to contact me for what ever you need ( i find out another two i think were last but i will push them after the first review )

[moozzyk]

I was trying your repro today and didn't seem to be able to get the same result. This is not to say that I don't believe there are leaks in the code. I received PRs fixing leaks in the past, but they were only piecemeal. I will need to run a more systematic analysis to review code that can leak. It feels like a time-consuming task, so it will take a while.

[kostasfilios]

now i see it, expand the SignalRClient (127 items) !!! this is the lib, your snapshot shows the VC ( app ) part
Should exist only one intance of each part of the lib due to we have closed 10 connections and only the last one have reference to VC ( until every connection closes it's normal to keep relevant instances due to URLSession keeps the refrence...that's why i added the last Dispatch async but ) it seems that we have 10 * each library part ( each class instance ) that's why there are 127 items ( instances of clasess ) in SignalRClient.
The leaks are in lib not in demo app (HubSamplePhone)

DispatchQueue.global().asyncAfter(deadline: .now() + 3) {
                        connection.stop()
                    }

[kostasfilios]

For example

Before leaks fix.
10 connections open-close with one refrence that VC keeps on chatHubConnection SIgnalRClient has 127 items:

50 connections open-close with one refrence that VC keeps on chatHubConnection SIgnalRClient has 617 items:

After leaks fix:
10 connections open-close with one refrence that VC keeps on chatHubConnection SIgnalRClient has 15 items:

100 connections open-close with one refrence that VC keeps on chatHubConnection SIgnalRClient has 15 items:

In memory leak case the rest of 15 items are zombie objects that cannot be accessed them on memory, because there is no reference on them and are retain cycles between stack and heap on iPhones memory due to ARC ( some classes reference count never will be 0 )
So in a normal flow where the app goes to background and the connection closes, app comes to foreground and start a new connection there is a zombie object of previous connection on memory, so in case where the user go back and forthe between background and foreground 10 times there are 112 zombie items in memory at SignalRClient