Skip to content

Commit 9f82520

Browse files
author
Clément Le Provost
committed
Fix memory leaks
Basically, the operations were retained by the completion blocks. They are now unowned, but to ensure that, I had to redesign a few things: - Operations are systematically enqueued to ensure that they are retained for their lifetime. I had to create dedicated queues for that. - The operation knows on which queue to dispatch the completion handler. This saves one level of indirection, thus avoiding a few references and above all ensuring consistency. The queue is most of the time the main queue, except for the offline sync. - Ensure that `finish()` is always called for every code path. (Otherwise the operation is never dequeued.)
1 parent bb12122 commit 9f82520

File tree

4 files changed

+58
-20
lines changed

4 files changed

+58
-20
lines changed

Source/Client.swift

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,15 @@ public let ErrorDomain = "AlgoliaSearch"
136136
// NOTE: Not constant only for the sake of mocking during unit tests.
137137
var session: URLSession
138138

139+
/// Operation queue used to keep track of requests.
140+
/// `Request` instances are inherently asynchronous, since they are merely wrappers around `NSURLSessionTask`.
141+
/// The sole purpose of the queue is to retain them for the duration of their execution!
142+
///
143+
let requestQueue: NSOperationQueue
144+
145+
/// Dispatch queue used to run completion handlers.
146+
private var completionQueue = dispatch_get_main_queue()
147+
139148
// MARK: Initialization
140149

141150
/// Create a new Algolia Search client.
@@ -172,6 +181,9 @@ public let ErrorDomain = "AlgoliaSearch"
172181
configuration.HTTPAdditionalHeaders = fixedHTTPHeaders
173182
session = NSURLSession(configuration: configuration)
174183

184+
requestQueue = NSOperationQueue()
185+
requestQueue.maxConcurrentOperationCount = 8
186+
175187
super.init()
176188

177189
// Add this library's version to the user agents.
@@ -376,18 +388,9 @@ public let ErrorDomain = "AlgoliaSearch"
376388
/// Perform an HTTP Query.
377389
func performHTTPQuery(path: String, method: HTTPMethod, body: [String: AnyObject]?, hostnames: [String], isSearchQuery: Bool = false, completionHandler: CompletionHandler? = nil) -> NSOperation {
378390
var request: Request!
379-
request = newRequest(method, path: path, body: body, hostnames: hostnames, isSearchQuery: isSearchQuery) {
380-
(content: [String: AnyObject]?, error: NSError?) -> Void in
381-
if completionHandler != nil {
382-
dispatch_async(dispatch_get_main_queue()) {
383-
// WARNING: Because of the asynchronous dispatch, the request could have been cancelled in the meantime.
384-
if !request.cancelled {
385-
completionHandler!(content: content, error: error)
386-
}
387-
}
388-
}
389-
}
390-
request.start()
391+
request = newRequest(method, path: path, body: body, hostnames: hostnames, isSearchQuery: isSearchQuery, completion: completionHandler)
392+
request.completionQueue = self.completionQueue
393+
requestQueue.addOperation(request)
391394
return request
392395
}
393396

Source/Offline/MirroredIndex.swift

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -513,8 +513,13 @@ import Foundation
513513
if index.requestStrategy == .FallbackOnTimeout && mayRunOfflineRequest {
514514
// Schedule an offline request to start after a certain delay.
515515
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, Int64(index.offlineFallbackTimeout * Double(NSEC_PER_SEC))), dispatch_get_main_queue()) {
516-
if self.mayRunOfflineRequest {
517-
self.startOffline()
516+
[weak self] in
517+
// WARNING: Because dispatched blocks cannot be cancelled, and to avoid increasing the lifetime of
518+
// the operation by the timeout delay, we do not retain self. => Gracefully fail if the operation
519+
// has already finished.
520+
guard let this = self else { return }
521+
if this.mayRunOfflineRequest {
522+
this.startOffline()
518523
}
519524
}
520525
}
@@ -526,6 +531,7 @@ import Foundation
526531
return
527532
}
528533
onlineRequest = startOnlineRequest({
534+
[unowned self] // works because the operation is enqueued and retained by the queue
529535
(content, error) in
530536
// In case of transient error, run an offline request.
531537
if error != nil && error!.isTransient() && self.mayRunOfflineRequest {
@@ -547,6 +553,7 @@ import Foundation
547553
return
548554
}
549555
offlineRequest = startOfflineRequest({
556+
[unowned self] // works because the operation is enqueued and retained by the queue
550557
(content, error) in
551558
self.onlineRequest?.cancel()
552559
self.callCompletion(content, error: error)
@@ -600,8 +607,7 @@ import Foundation
600607
else {
601608
let queryCopy = Query(copy: query)
602609
let operation = OnlineOfflineSearchOperation(index: self, query: queryCopy, completionHandler: completionHandler)
603-
// NOTE: This operation is just an aggregate, so it does not need to be enqueued.
604-
operation.start()
610+
offlineClient.mixedRequestQueue.addOperation(operation)
605611
return operation
606612
}
607613
}
@@ -693,7 +699,7 @@ import Foundation
693699
// A mirrored index launches a mixed offline/online request.
694700
else {
695701
let operation = OnlineOfflineMultipleQueriesOperation(index: self, queries: queries, completionHandler: completionHandler)
696-
operation.start()
702+
offlineClient.mixedRequestQueue.addOperation(operation)
697703
return operation
698704
}
699705
}

Source/Offline/OfflineClient.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,10 @@ import Foundation
4343
buildQueue.maxConcurrentOperationCount = 1
4444
searchQueue.name = "AlgoliaSearch-Search"
4545
searchQueue.maxConcurrentOperationCount = 1
46+
mixedRequestQueue.name = "AlgoliaSearch-Mixed"
4647
rootDataDir = NSSearchPathForDirectoriesInDomains(.ApplicationSupportDirectory, .UserDomainMask, true).first! + "/algolia"
4748
super.init(appID: appID, apiKey: apiKey)
49+
mixedRequestQueue.maxConcurrentOperationCount = super.requestQueue.maxConcurrentOperationCount
4850
userAgents.append(LibraryVersion(name: "AlgoliaSearchOfflineCore-iOS", version: sdk.versionString))
4951
}
5052

@@ -69,6 +71,13 @@ import Foundation
6971

7072
/// Queue used to search local indices in the background.
7173
let searchQueue = NSOperationQueue()
74+
75+
/// Queue for mixed online/offline operations.
76+
///
77+
/// + Note: We could use `Client.requestQueue`, but since mixed operations are essentially aggregations of
78+
/// individual operations, we wish to avoid deadlocks.
79+
///
80+
let mixedRequestQueue = NSOperationQueue()
7281

7382
/// Enable the offline mode.
7483
///

Source/Request.swift

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ class Request: AsyncOperation {
6565
/// User completion block to be called.
6666
let completion: CompletionHandler?
6767

68+
/// Dispatch queue used to execute the completion handler.
69+
var completionQueue: dispatch_queue_t?
70+
6871
// MARK: - Initialization
6972

7073
init(session: URLSession, method: HTTPMethod, hosts: [String], firstHostIndex: Int, path: String, headers: [String: String]?, jsonBody: [String: AnyObject]?, timeout: NSTimeInterval, completion: CompletionHandler?) {
@@ -193,10 +196,27 @@ class Request: AsyncOperation {
193196
/// Finish this operation.
194197
/// This method should be called exactly once per operation.
195198
private func callCompletion(content: [String: AnyObject]?, error: NSError?) {
196-
if completion != nil && !_cancelled {
197-
completion!(content: content, error: error)
199+
if _cancelled {
200+
return
201+
}
202+
if let completionQueue = completionQueue {
203+
dispatch_async(completionQueue) {
204+
self._callCompletion(content, error: error)
205+
}
206+
} else {
207+
_callCompletion(content, error: error)
208+
}
209+
}
210+
211+
private func _callCompletion(content: [String: AnyObject]?, error: NSError?) {
212+
// WARNING: In case of asynchronous dispatch, the request could have been cancelled in the meantime
213+
// => check again.
214+
if !_cancelled {
215+
if let completion = completion {
216+
completion(content: content, error: error)
217+
}
218+
finish()
198219
}
199-
finish()
200220
}
201221

202222
// ----------------------------------------------------------------------

0 commit comments

Comments
 (0)