Alamofire: Possible memory leak adding many background download requests in a loop

[unusuallyreticent]

Excuse the last post - accidentally hit something...

I have a strange issue with Instruments leaks. I'm running a loop to add many download requests to a background session. The SessionManager is contained in a class singleton pretty much as follows:

class BackgroundTransferCoordinator {
static let sharedInstance = BackgroundTransferCoordinator()
lazy var session: SessionManager = {
// session with background configuration
}()
// .. too much to show
}

When building background requests in a loop, the following code results in a stunning number of leaks after adding just four requests:

for _ in 0..<4 {
let request = self.session.download(url)
request.resume()
}

To confuse matters, adding less than 4 items is more than often OK. Replace the for loop content with this:

let request = self.session.session.downloadTask(with: url)
request.resume()

...and you can add as any requests as you like, with no leaks showing up. I'm typically adding 50 or more requests on a first run and then adding a couple per day thereafter - documents.

The BackgroundTransferCoordinator class sets handlers for:
session.delegate.taskDidComplete
session.delegate.downloadDidFinishDownloadingToURL

I actually run the code inside the getAllTasks completion handler to query existing background tasks and then add missing tasks. However, other code builds requests on the main queue and that also fails after adding more than three in a loop. There is no crash occurring, and all requests complete properly - got the files to prove it.

As far as I can see, the difference appears to be the second example, by using the URLSession directly, bypasses this:

private func download( _ downloadable: DownloadRequest.Downloadable,
to destination: DownloadRequest.DownloadFileDestination?)
-> DownloadRequest

... and requests do not end up being added to SessionDelegate.requests

I'm not convinced this isn't an issue with Instruments or the way I'm using it. I will try to make a test project in the next couple of days if possible.

[jshier]

Can you post the leaks output or the lines it points to as responsible? While a demo project would be appreciated, being able to examine the leaks you've seen would be helpful in the meantime.

[unusuallyreticent]

I have attached a couple of zipped CSV files. I have also confirmed the issue exists adding requests in a loop though I have no clear idea yet why. The following code block shows the problem code. It is running inside the getAllTasks completionHandler but has exactly the same issue running on the main queue.

IQSMS-Reporting-leaksWithLoopIssue.csv.zip
IQSMS-Reporting-leaksNoLoopIssue.csv.zip

[unusuallyreticent]

OK, safari is not playing ball with my code when using // comments. The following code picks up unreadable folder files and starts a background download. The TransferID().string is a short encoded description string used to identify the task later. I'm not keeping any references to download tasks. Session.startsRequestsImmediately is false.

        if completed < total {
            let unreachable = folder.manifest.unreachable
            for rec in unreachable {
                let taskDescription = TransferID(type: .update, stagingId: stagingId, name: rec.filename, md5sum: rec.md5sum).string
                if taskDescriptions.contains(taskDescription) == false {

                    /*
                    let req = self.session.download(rec.url)
                    req.task?.taskDescription = taskDescription
                    req.resume()
                     */
                    let req = self.session.session.downloadTask(with: rec.url)
                    req.taskDescription = taskDescription
                    req.resume()

                }

            }
        } else if total > 0 {
            self.stagingID = 0
            _ = folder.convertToReady()
        }

[unusuallyreticent]

There are 20 files in the code loop shown previously. I only have to loop for 4 to get the leaks trace. Each time the code run is repeated (deleting the folder of files), total memory used grows, so I'm beginning to think block is being retained somewhere.

[unusuallyreticent]

[unusuallyreticent]

The loop code without the issue produces this image. I clicked refresh 10 times in the trace period. The error message on the App is because I faked an md5 checksum error on the last file in order to loop the test.

[unusuallyreticent]

Also, I forgot to mention, there are 2 x SessionManagers running. The default is doing the foreground fetches very nicely thank you :-). That should explain why there are 2 x NSLock objects shown.

[jshier]

The lock leaks are a false positive. As for the background leaks, I'm not sure what's happening there, but it looks like you're leaking the world almost. If you can get a test project to us that would greatly help with our investigation.

[unusuallyreticent]

I'll post a test later today.

[unusuallyreticent]

AlamofireBackgroundSessionTest.zip

One test project!

[unusuallyreticent]

Do try setting the loop to 0..<4

[unusuallyreticent]

Something is bit odd. In the test project SessionTest, unwire taskDidComplete. Note the function is empty in the test. Just wiring it up causes the issue in Instruments Leaks.

init() {
   // session.delegate.taskDidComplete = self.backgroundSessionTaskDidComplete
   session.delegate.downloadTaskDidFinishDownloadingToURL = self.downloadTaskDidFinishDownloadingToURL
}

[unusuallyreticent]

Ok guys, bit of code for you to look at. The mod is not in the best place - should be in the if taskDidComplete block but I think you will get the idea as to what is going wrong if session.taskDidComplete is wired up:

open func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) {
    /// Executed after it is determined that the request is not going to be retried
    let completeTask: (URLSession, URLSessionTask, Error?) -> Void = { [weak self] session, task, error in
        guard let strongSelf = self else { return }

        if let taskDidComplete = strongSelf.taskDidComplete {
            taskDidComplete(session, task, error)
        } else if let delegate = strongSelf[task]?.delegate {
            delegate.urlSession(session, task: task, didCompleteWithError: error)
        }

        NotificationCenter.default.post(
            name: Notification.Name.Task.DidComplete,
            object: strongSelf,
            userInfo: [Notification.Key.Task: task]
        )

        // ADDED. delegate queue is not resumed if only Session.taskDidComplete is wired!
        if let delegate = strongSelf[task]?.delegate {
            delegate.queue.isSuspended = false
        }

        strongSelf[task] = nil
    }

    guard let request = self[task], let sessionManager = sessionManager else {
        completeTask(session, task, error)
        return
    }

    // Run all validations on the request before checking if an error occurred
    request.validations.forEach { $0() }

    // Determine whether an error has occurred
    var error: Error? = error

    if let taskDelegate = self[task]?.delegate, taskDelegate.error != nil {
        error = taskDelegate.error
    }

    /// If an error occurred and the retrier is set, asynchronously ask the retrier if the request
    /// should be retried. Otherwise, complete the task by notifying the task delegate.
    if let retrier = retrier, let error = error {
        retrier.should(sessionManager, retry: request, with: error) { [weak self] shouldRetry, timeDelay in
            guard shouldRetry else { completeTask(session, task, error) ; return }

            DispatchQueue.utility.after(timeDelay) { [weak self] in
                guard let strongSelf = self else { return }

                let retrySucceeded = strongSelf.sessionManager?.retry(request) ?? false

                if retrySucceeded, let task = request.task {
                    strongSelf[task] = request
                    return
                } else {
                    completeTask(session, task, error)
                }
            }
        }
    } else {
        completeTask(session, task, error)
    }
}

[unusuallyreticent]

Oh, I forgot to mention, if you just move the 'private let lock = NSLock()' right to the top of the SessionDelegate class, you want see this anymore in Leaks. It's a weird thing in Swift.

open class SessionDelegate: NSObject {
private let lock = NSLock() // <-- first in the class

[unusuallyreticent]

damn your spell checker! "you won't see ..."

[chamira]

The issue looks different from simulator to device. I ran the same code both on device and the simulator they look different. (Leak object) But when I look at the stack trace on both SessionDelegate.init() can be seen.

@unusuallyreticent

Oh, I forgot to mention, if you just move the 'private let lock = NSLock()' right to the top of the SessionDelegate class, you want see this anymore in Leaks. It's a weird thing in Swift.

I tried it but nothing changed.

[unusuallyreticent]

The Request adds a block operation referencing self, to the TaskDelegate.queue. I can't remember what it is off-hand, something to do with setting a time maybe. The taskDelegate is itself a strong property of the Request. The taskDelegate queue is not resumed in two situations. The first is when SessionDelegate.taskDidComplete is wired up. The second is when taskDelegate.TaskDidCompleteWithError is wired up. I am not 100% sure of the second case, however the first I can see clearly in debug. The queue is never resumed and the whole request and task delegate leak due to a strong reference cycle held inside that queue. At least, that's what I think it is. I not the guru here. I just make the tea.

The last cleanup point is inside the completeTask block inside the SessionDelegate didCompleteWithError function I listed earlier. The code modification I made ensures that queue is resumed, the queue operation completes and the reference cycle is remove:

if let delegate = strongSelf[task]?.delegate {
delegate.queue.isSuspended = false
}

Request.resume() has this, but task is NOT nil so it does not get executed in this case...
guard let task = task else { delegate.queue.isSuspended = false ; return }

The only other place where isSuspended is set false is in TaskDelegate didCompleteWithError. That bit of code is never run if SessionDelegate.taskDidComplete is wired up. If taskDidComplete is not wired up and TaskDelegate.taskDidCompleteWithError is wired up instead, then again, the queue is never resumed and the request (and all its references) and the task delegate leak.

The separate issue of the NSLock() appearing in instruments is I believe, due to a bit of a mess with swift initialisation. It seems to occur in static instance classes. If you want something really odd, try adding a couple of string dictionaries to the the AppDelegate. I guess it has something to do with the way Instruments works but, silly as it sounds, repositioning properties makes a difference. It can of course, work both ways. Repositioning can make something else appear to leak. Its just something I discovered by accident.

[unusuallyreticent]

AlamofireBackgroundSessionTest.zip

Here is the same test project with the SessionDelegate file modification in urlSession-didCompleteWithError, and the repositioned NSLock() just to make Instruments-leaks show pretty green lights.

[unusuallyreticent]

@chamira, the NSLock() property has to be first - before any other property. Look what I did to SessionDelegate in the test project. I only moved this single property to the top. You have moved retrier, sessionManager and requests as well. Leave them were they were. It's a really silly issue. Laughable really.

[jshier]

@unusuallyreticent Thanks for the investigation, it helped narrow this down quite a bit. It turns out that, when the taskDidComplete closure is set, AF isn't calling it's own TaskDelegate, which means we lose all of the machinery in there that takes care of task completion. So not only would it leak but any response handlers attached would never be called. I've put up a tentative fix on the bug/1938 branch, pull it down with CocoaPods and see if it works for you. I still need to add some tests but it should fix the issues you're seeing.

[unusuallyreticent]

@jshier - thanks. I'll try it out. And, sorry if I seemed a bit rude occasionally. It was unintentional - late nights, three projects on the go, too much coffee :-(

I was only looking at my particular corner case - using taskDidComplete so I didn't consider that TaskDelegate should still be called anyway. Makes sense. They aren't exclusive.

Did you try that silly trick with NSLock()? That has got to be the daftest trick. If anyone told me to do that I would consider them loony.

[unusuallyreticent]

@jshier - that's solved it.

[cnoon]

Hey everyone,

Sorry for being late to the party here. @jshier and I discussed the changes that he put up in the bug/1938 branch. I'm not surprised that fixed the issue for you @unusuallyreticent. You have to be very careful right now with the session override closures to make sure you don't shut off some of the functionality.

The change that @jshier made is switching that taskDidComplete closure over to a hook instead of an override. After quite a bit of debate, we realized that there shouldn't be any harm in this change since it was impossible to get the queue resumed without a huge amount of work. Therefore we're fairly confident this change won't break any clients out there now.

We're going to look to unify the override closures in AF5 or just remove them completely with the goal being to make sure their usage is clear and not easy to mess up. There is enough inconsistency right now that it can lead to issues like you experienced.

As for the NSLock leak, I've actually filed a radar against that specific leak and it was closed as a duplicate. That is not something we can do anything about right now unfortunately.

I've pushed up @jshier's bug fix into the master branch in d64afca3 and it will go out as part of the Alamofire 4.4.0 release here shortly.

Cheers. 🍻