Is [Weak self] always needed when dealing with URLSession?

Question

I can't figure out if I need to use [weak self] in this situation or not ?

HTTPClient.swift:

struct HTTPClient {
    let session = URLSession.shared

    func get(url: URL, completion: @escaping (Data) -> Void) {
        session.dataTask(with: url) { data, urlResponse, error in
        completion(data) // assume everything will go well
      }.resume()
    }
}

Service.swift

struct Service {
    let httpClient: HTTPClient

    init(httpClient: HTTPClient = HTTPClient()) {
        self.httpClient = httpClient
    }

    func fetchUser(completion: @escaping (User) -> Void) {
        httpClient.get("urlToGetUser") { data in
          // transform data to User 
          completion(user)
        } 
    }
}

ViewModel.swift

class ViewModel {
   let service: Service
   let user: User?
   var didLoadData: ((User) -> Void)?

    init(service: Service) {
        self.service = service
        loadUser()
    }

   func loadUser() {
     service.fetchUser { [weak self] user in // is  [weak self] really needed ?
            self?.user = user
            self?.didLoadData?(user)
        }
   }
}

is it really needed here to user [weak self] ? and is there a rule on how to check if it's needed in general when we deal with an API that we don't know what's happening to the closure ? or that does not matter (it's up to us to decide) ?

Answer 1

In the example you've given, [weak self] is potentially unnecessary. It depends on what you want to happen if ViewModel is released before the request completes.

As noted in the URLSessionDataTask docs (emphasis mine):

After you create a task, you start it by calling its resume() method. The session then maintains a strong reference to the task until the request finishes or fails; you don’t need to maintain a reference to the task unless it’s useful for your app’s internal bookkeeping.

The session has a strong reference to the task. The task has a strong reference to the closure. The closure has a strong reference to the ViewModel. As long as the ViewModel doesn't have a strong reference to the task (which it doesn't in the code you provided), then there's no cycle.

The question is whether you want to ensure that the ViewModel continues to exist long enough for the closure to execute. If you do (or don't care), then you can use a simple strong reference. If you want to prevent the task from keeping the ViewModel alive, then you should use a weak reference.

This is how you need to think about reference cycles. There is no general rule "use weak here." You use weak when that's what you mean; when you don't want this closure to keep self around until it is released. That particularly is true if it creates a cycle. But there's no general answer for "does this create a cycle." It depends on what pieces hold references.

This also points to where your current API design is not as good as it could be. You're passing didLoadData in init. That very likely is going to create reference cycles and force your caller to use weak. If instead you made didLoadDdata a completion handler on loadUser(), then you could avoid that problem and make life easier on the caller.

func loadUser(completion: @escaping ((User?) -> Void)? = nil) {
    service.fetchUser {
        self?.user = user
        didLoadData?(user)
    }
}

(Your current API has a race condition in any case. You're starting loadUser() before didLoadData can be set. You may be assuming that the completion handler won't complete before you've set dataDidLoad, but there's no real promise of that. It's probably true, but it's fragile at best.)

Answer 2

The problem with your code is not with the use of URLSession but with the fact that you are retaining a function in your view controller:

class ViewModel {
    var didLoadData: ((User) -> Void)?
}

If the didLoadData function mentions self (i.e. the ViewModel instance) implicitly or explicitly, you have a retain cycle and a memory leak unless you say weak self or unowned self.