Completion handler never called

1
votes

I have a function like this. 

class func getAccountBalances(completionHandler:@escaping ((_ balances:Any) -> Void)){
//Alamofire request and we get the result. But sometimes the result fails.
    switch response.result {
    case .success(let value):
        completionHandler(value)
    case .failure(let error):
        print ("error is: \(error)")
}

I am not putting code to handle the result if it fails. Is that a bad thing? Do I need to have a completion handler in the case that the call fails so that this function does not stay in the memory waiting for that completion handler to be called? What is the best practice?

2
swiftswift4
What type of object is your response.result and does it return true for .success(response.result) in a test? - CStreel
JSON and normally it does return true but sometimes it fails. I am using codable to parse it. - Nevin Jethmalani
@NevinJethmalani add a second parameter (Error?) to your completion handler. Day you really need to return Any ? If so change it to (Any?, Error?) otherwise better to return the correct object type Balance? - Leo Dabus
There are two considerations: If there's a risk of a strong reference cycle (where you store this closure as a parameter and never release it), then absolutely use [weak self]. But that's a non-issue with Alamofire, because they release the closure when they're done with it. (Look on the Alamofire page and they rarely/never use [weak self] pattern, because it's simply not needed.) The other concern is more trivial: If you issue a request and happen to dismiss the view controller before request finishes, do you care if it hangs on to the view controller until the request finishes or not? - Rob
... Generally you don't care, so you don't bother with [weak self]. But if you do (e.g. if the view controller takes up a lot of memory or if the request could likely be very slow), then by all means you can use [weak self] to let the view controller be deallocated as soon as its dismissed. So Radu is right that you can use [weak self] pattern, but in the case of Alamofire, it's often a non-issue. And I certainly didn't want you to be worried about using [weak self] to prevent leaks in this particular case. But you can use that pattern if you want. - Rob

2 Answers

1
votes

In general, it is a good practice to call the completion on every case. The reason for this is that you usually want to let the upper lever(business logic layer) decide if it should mark some balances(for example) as saved, or maybe show a dialog when an error has occurred. That is a good practice with the thinking that everything should be a module. That being said, if another module will want to call the same function at some point, it may be a good thinking to let that module what happened with the result. That can be implemented in several ways, I won't enter here, it's your decision.

However, it's not a must to do it. If a block won't be called it should be deallocated, and then everything is good memory-wise. So in your example, if you don't retain the block somewhere else(for example holding it in a variable inside the class that makes getAccountBalances call), you should be just fine.

Another important part is when you call the function be careful to not create a memory leak where you retain the self inside the block:

getAccountBalances() { _ in 
   self.updateUI()
}

This block will create a retain to self and if everything goes okay with the call, but user left the screen, you may end up using variables that were deallocated and crash the app. A good practice here is to not retain the self in the callback, but make it weak before that:

getAccountBalances() { [weak self] _ in 
   // This is not necessarily needed, and you could use self?.updateUI() instead. 
   // However, this is usually another thing i like to do and consider it a good practice
   guard let `self` = self else { return }
   self.updateUI()
}
0
votes

Your title: Completion handler never called

Your code: does not return any value to the completion handler if there is an error.

How would you expect to see a result of the competion handler if there is a failure? Would you like to crash the app? This way is better because it handles both cases:

class func getAccountBalances(completionHandler:@escaping ((_ balances:Any?) -> Void)){ //make Any an optional
//Alamofire request and we get the result. But sometimes the result fails.
    switch response.result {
    case .success(let value):
        completionHandler(value)
    case .failure(let error):
        print ("error is: \(error)")
        completionHandler(nil)
}

Your new call to this function:

getAccountBalances() { value in 
guard let _value = value else { // anticipate on the error return }
// use _value, the balances are inside.
}

An other approach would be not making it nil, but downcasting the value inside of it. That would look like this:

class func getAccountBalances(completionHandler:@escaping ((_ balances:Any) -> Void)){
//Alamofire request and we get the result. But sometimes the result fails.
    switch response.result {
    case .success(let value):
        completionHandler(value)
    case .failure(let error):
        print ("error is: \(error)")
        completionHandler(error) //notice this change
}

Then your function would look like this:

getAccountBalances() { value in 
if let error = value as? Error { //or whatever type your error is in the function
  //there is an error
}
}