Skip to content

Commit 69e5992

Browse files
author
Clément Le Provost
committed
Fix race condition when searching while activating mirrored mode
Although instantiation of `MirroredIndex.localIndex` was conditioned to setting `MirroredIndex.mirrored` to true, and although `mirrored`’s accessors were synchronized, the instantiation itself was not part of the critical section. This led to a race condition when `search(…)` is called from another thread after `mirrored` has been set but before `localIndex` has been instantiated. NOTE: I had to give up deallocating the local index when setting `mirrored` back to false, because mixed operations or a sync may still be ongoing. Keeping it alive until all operations have completed would be extremely complicated and not worth it, because there is anyway little point in disabling mirrored mode once it has been activated. This flag is merely so that purely online indices can still be used with an `OfflineClient`. Fixes #285.
1 parent 067bb66 commit 69e5992

File tree

3 files changed

+64
-15
lines changed

3 files changed

+64
-15
lines changed

Source/Helpers.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,12 +128,12 @@ extension NSObject {
128128
///
129129
/// - paremeter block: Block to be executed serially on the object.
130130
///
131-
func synchronized(_ block: () -> ()) {
131+
func synchronized<T>(_ block: () -> T) -> T {
132132
objc_sync_enter(self)
133133
defer {
134134
objc_sync_exit(self)
135135
}
136-
block()
136+
return block()
137137
}
138138
}
139139

Source/Offline/MirroredIndex.swift

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -196,30 +196,41 @@ import Foundation
196196
let mirrorSettings = MirrorSettings()
197197

198198
/// Whether the index is mirrored locally. Default = false.
199-
@objc public var mirrored: Bool = false {
199+
///
200+
/// + Note: Setting this property to `true` create the resources required to access the local mirror.
201+
/// Setting it to `false` does *not* deallocate those resources.
202+
///
203+
@objc public var mirrored: Bool {
204+
get {
205+
return self.synchronized { self._mirrored }
206+
}
207+
set {
208+
self.synchronized { self._mirrored = newValue }
209+
}
210+
}
211+
212+
/// Storage for the `mirrored` property.
213+
///
214+
/// + Warning: **DO NOT** ACCESS THIS VARIABLE DIRECTLY.
215+
///
216+
private var _mirrored: Bool = false {
217+
// NOTE: Synchronization performed by the `mirrored` property.
200218
didSet {
201-
if (mirrored) {
219+
if (_mirrored) {
202220
do {
203221
try FileManager.default.createDirectory(atPath: self.indexDataDir, withIntermediateDirectories: true, attributes: nil)
204222
// Lazy instantiate the local index.
205-
self.synchronized {
206-
if (self.localIndex == nil) {
207-
self.localIndex = LocalIndex(dataDir: self.offlineClient.rootDataDir, appID: self.client.appID, indexName: self.name)
208-
}
223+
if (self.localIndex == nil) {
224+
self.localIndex = LocalIndex(dataDir: self.offlineClient.rootDataDir, appID: self.client.appID, indexName: self.name)
209225
}
210226
} catch _ {
211227
// Ignore
212228
}
213229
mirrorSettings.load(self.mirrorSettingsFilePath)
214-
} else {
215-
// Release the local index.
216-
self.synchronized {
217-
self.localIndex = nil
218-
}
219230
}
220231
}
221232
}
222-
233+
223234
/// Data selection queries.
224235
@objc public var dataSelectionQueries: [DataSelectionQuery] {
225236
get {
@@ -646,7 +657,6 @@ import Foundation
646657
private var mayRunOfflineRequest: Bool = true
647658

648659
init(index: MirroredIndex, completionHandler: @escaping CompletionHandler) {
649-
assert(index.mirrored)
650660
self.index = index
651661
super.init(completionHandler: completionHandler)
652662
self.completionQueue = index.client.completionQueue

Tests/Offline/MirroredIndexTests.swift

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,4 +586,43 @@ class MirroredIndexTests: OfflineTestCase {
586586

587587
waitForExpectations(timeout: onlineExpectationTimeout, handler: nil)
588588
}
589+
590+
/// Test that we can switch an index from not mirrored to mirrored and back without causing any harm.
591+
///
592+
/// + Bug: [#285](https://github.com/algolia/algoliasearch-client-swift/issues/285).
593+
///
594+
func testMirroredSetUnset() {
595+
let expectation_property = self.expectation(description: #function + " (property)")
596+
let expectation_search = self.expectation(description: #function + " (search)")
597+
598+
let index: MirroredIndex = client.index(withName: safeIndexName(#function))
599+
600+
// NOTE: We don't care about the search results here. We are just interested in not crashing.
601+
//
602+
let propertyQueue = DispatchQueue(label: "Property queue")
603+
let OUTER_ITERATIONS = 100
604+
let INNER_ITERATIONS = 100
605+
var completedProperties = 0
606+
var completedSearches = 0
607+
for _ in 1...OUTER_ITERATIONS {
608+
propertyQueue.async {
609+
for i in 1...INNER_ITERATIONS {
610+
index.mirrored = i % 2 == 1
611+
}
612+
completedProperties += 1
613+
if completedProperties == OUTER_ITERATIONS {
614+
expectation_property.fulfill()
615+
}
616+
}
617+
for _ in 1...1 {
618+
index.search(Query()) { (content, error) in
619+
completedSearches += 1
620+
if completedSearches == OUTER_ITERATIONS {
621+
expectation_search.fulfill()
622+
}
623+
}
624+
}
625+
}
626+
waitForExpectations(timeout: onlineExpectationTimeout, handler: nil)
627+
}
589628
}

0 commit comments

Comments
 (0)