Alamofire 4.0.0 memory leaks with uploadProgress and downloadProgress

[StephaneGarnier]

Hi,

I think that I have identified a memory leak when I am using uploadProgress or downloadProgress in my code.

Here is the code example that is generating memory leak.

func sendcapturedPhoto(_ id: String, capturedPhoto: UIImage, progress: @escaping ((_ progressValue: Progress) -> Void), completed: @escaping ((_ success: Bool, _ contributionId: String?, _ error: AppError?) -> Void)) -> Alamofire.Request {

        var endPoint = Router.addImageToSweebi(id: id)

        return Alamofire.upload(UIImageJPEGRepresentation(capturedPhoto, 0.8)!, with: endPoint)
            .validate()
            .uploadProgress(closure: { (prog) in
                progress(prog)
            })
            .responseOMObject { (request, success, data: Image?, error) in
                completed(success, data?.id, error)
        }
    }

If I don't use uploadProgress I don't have memory leaks and all my objects are correctly deinit. But I want to know the progression of an upload.
I find a way to avoid this problem in Alamofire. In UploadRequest I have replaced uploadProgress with this one:

@discardableResult
    open func uploadProgress(queue: DispatchQueue = DispatchQueue.main, closure: @escaping ProgressHandler) -> Self {
        uploadDelegate.uploadProgressHandler = ({(progress) in
            closure(self.uploadProgress)
            if progress.fractionCompleted == 1.0 {
                self.uploadDelegate.uploadProgressHandler = nil
            }
            }, queue)
        return self
    }

After that, memory leaks disappeared. I think the same thing could be done with downloadProgress.

I want to open issue on this because I am sure that my solution is not the the good one.

[LaurentLK]

I've encountered a memory leak in AlamofireImage and found the leak is linked to the Request.validate() method. If I remove the validate() the memory leak disappears.

[asynchrony-ringo]

The validate() leaks appear related to Validate.swift building closures while capturing strong self... e.g. lines 165, 241. These need changing to unowned self or refactored for weak self

For example: Around Validation.swift Line 165

let validationExecution: () -> Void = {
            if
                let response = self.response,
                self.delegate.error == nil,
                case let .failure(error) = validation(self.request, response, self.delegate.data)
            {
                self.delegate.error = error
            }
        }

Suggested change:

let validationExecution: () -> Void = {  [unowned self] in 
            if
                let response = self.response,
                self.delegate.error == nil,
                case let .failure(error) = validation(self.request, response, self.delegate.data)
            {
                self.delegate.error = error
            }
        }

There are several of these throughout the code that need to be handled.

[cnoon]

Thank you everyone for all the detailed information.

@asynchrony-ringo is certainly correct. The core issue stemmed from the validation closures not marking each internally stored closure as unowned self. I've pushed 32b0ae9 into master which will be released shortly that addresses the issue. Validation no longer causes the request to leak. Also @StephaneGarnier, the download and upload progress APIs do not cause any leaks.

Background

To give a bit of backstory, any closure that executes on the TaskDelegate's operation queue MUST capture a reference to self. Without this, the Request will actually be deallocated before the closure can be run which will result in no-ops for things like response handlers. The reason for this is that once the URLSessionTask completes, Alamofire no longer holds a strong reference to the Request. It is released by the SessionDelegate as soon as the task completes.

This behavior is slightly different when using a RequestRetrier, but let's assume for now you're not using one.

Once the task completes, the Request's internal operation queue is resumed and the Request is released by Alamofire. The only thing still holding a reference to the Request at this point is any closure that was dispatched onto the operation queue. GCD holds onto the operation queue (dispatch queue underneath) until all the closures on the queue have completed. Then GCD will cleanup the queue which is roughly the same time the Request will be deallocated.

Alamofire 4

In Alamofire 4, we moved all validation to be stored directly by the Request so we can run validations off the operation queue. This allows us to support the RequestRetrier before actually starting the internal operation queue in case we need to retry the request before running the response handlers. The mistake we made when we did this was that we forgot to mark the validation closures as unowned so that we wouldn't create a retain cycle. The changes in 32b0ae9 remove the retain cycle.

We'll look to get this fix deployed as soon as possible. Thanks again everyone for all your help here.

Cheers. 🍻

[rocxteady]

The problem still exists. I am getting leaks If I validate the requests. The leak is gone If I remove the validation part.