From 6a4399c6dac34834d3c5683b4aa7acf483101705 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Thu, 16 May 2024 08:41:42 +0200 Subject: [PATCH 1/8] [chore] Fix data race in PersistentFileLogSpec Synchronize access to bool value with NSLock --- .../ios/Tests/PersistentFileLogSpec.swift | 48 +++++++++++++++---- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift index 29d14abb55f40..e1c976ca890ed 100644 --- a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift +++ b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift @@ -48,26 +48,56 @@ class PersistentFileLogSpec: ExpoSpec { } static func clearEntriesSync(log: PersistentFileLog) { - var didClear = false + let didClear = SynchronizedBool(false) log.clearEntries { _ in - didClear = true + didClear.value = true } - expect(didClear).toEventually(beTrue(), timeout: .milliseconds(500)) + // the awaiter reads on another thread such that didClear is set on another queue + expect(didClear.value).toEventually(beTrue(), timeout: .milliseconds(500)) } static func filterEntriesSync(log: PersistentFileLog, filter: @escaping PersistentFileLogFilter) { - var didPurge = false + let didPurge = SynchronizedBool(false) log.purgeEntriesNotMatchingFilter(filter: filter) { _ in - didPurge = true + didPurge.value = true } - expect(didPurge).toEventually(beTrue(), timeout: .milliseconds(500)) + expect(didPurge.value).toEventually(beTrue(), timeout: .milliseconds(500)) } static func appendEntrySync(log: PersistentFileLog, entry: String) { - var didAppend = false + let didAppend = SynchronizedBool(false) log.appendEntry(entry: entry) { _ in - didAppend = true + didAppend.value = true } - expect(didAppend).toEventually(beTrue(), timeout: .milliseconds(500)) + expect(didAppend.value).toEventually(beTrue(), timeout: .milliseconds(500)) } } + +private final class SynchronizedBool { + private var _storage: Bool + private var lock = NSLock() + + var value: Bool { + get { + return lockAround { + _storage + } + } + set { + lockAround { + _storage = newValue + } + } + } + + init(_ storage: Bool) { + self._storage = storage + } + + private func lockAround(_ closure: () -> T) -> T { + lock.lock() + defer { lock.unlock() } + return closure() + } + +} From 1794e2ca093ca70ac53cece3986c39392688bb10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Thu, 16 May 2024 15:48:55 +0200 Subject: [PATCH 2/8] =?UTF-8?q?[chore]=C2=A0add=20documentation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../expo-modules-core/ios/Tests/PersistentFileLogSpec.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift index e1c976ca890ed..fa88eb52439f6 100644 --- a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift +++ b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift @@ -52,7 +52,6 @@ class PersistentFileLogSpec: ExpoSpec { log.clearEntries { _ in didClear.value = true } - // the awaiter reads on another thread such that didClear is set on another queue expect(didClear.value).toEventually(beTrue(), timeout: .milliseconds(500)) } @@ -73,10 +72,14 @@ class PersistentFileLogSpec: ExpoSpec { } } +/// Since the boolean value is written to in the closure supplied to `log.appendEntry`, +/// while concurrenlty being read in `expect`, i.e., via the autoclosure, synchronization +/// is needed to avoid data races. This class allows for simple synchronization of a boolean value. private final class SynchronizedBool { private var _storage: Bool private var lock = NSLock() + /// Thread safe access here. var value: Bool { get { return lockAround { From 27d0718744cea4a8fa2edc8f71e3607922b74a4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Thu, 16 May 2024 15:58:54 +0200 Subject: [PATCH 3/8] =?UTF-8?q?[chore]=C2=A0add=20changelog?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/expo-modules-core/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/expo-modules-core/CHANGELOG.md b/packages/expo-modules-core/CHANGELOG.md index d11dbe14578dc..9a9c34b1bb2d3 100644 --- a/packages/expo-modules-core/CHANGELOG.md +++ b/packages/expo-modules-core/CHANGELOG.md @@ -8,6 +8,8 @@ ### 🐛 Bug fixes +- Fix data race in `PersistentFileLogSpec.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) + ### 💡 Others ## 1.12.11 — 2024-05-14 From 6552c9dabdae2547a1ed4842cbdbd4c009d8d83b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Sun, 19 May 2024 12:48:41 +0200 Subject: [PATCH 4/8] Refactor and add sync to AppLauncherWithDatabaseMock --- .../Tests/EXDevLauncherURLHelperTests.swift | 4 +- .../Tests/AppLauncherWithDatabaseSpec.swift | 39 +++++++++++++++++-- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift b/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift index a5fb14560ad51..53d49538c165d 100644 --- a/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift +++ b/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift @@ -24,8 +24,8 @@ class EXDevLauncherURLHelperTests: XCTestCase { XCTAssertEqual(URL(string: "http://expo-development-client/?url=http%3A%2F%2Flocalhost%3A8081"), actual2) // should not crash if provided URL does not include scheme - let actual3 = EXDevLauncherURLHelper.replaceEXPScheme(URL(string: "192.168.0.12:8081")!, to: "scheme") - XCTAssertEqual(URL(string: "192.168.0.12:8081"), actual3) +// let actual3 = EXDevLauncherURLHelper.replaceEXPScheme(URL(string: "192.168.0.12:8081")!, to: "scheme") +// XCTAssertEqual(URL(string: "192.168.0.12:8081"), actual3) } func testDevLauncherUrls() { diff --git a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift index 9bab981451909..f95a388c7d589 100644 --- a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift +++ b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift @@ -94,16 +94,16 @@ class AppLauncherWithDatabaseSpec : ExpoSpec { directory: testDatabaseDir, completionQueue: DispatchQueue.global(qos: .default) ) - var successValue: Bool? = nil + let successValue = Synchronized(nil) launcher.launchUpdate(withSelectionPolicy: SelectionPolicyFactory.filterAwarePolicy(withRuntimeVersion: "1")) { error, success in - successValue = success + successValue.value = success } - while successValue == nil { + while successValue.value == nil { Thread.sleep(forTimeInterval: 0.1) } - expect(successValue) == true + expect(successValue.value) == true db.databaseQueue.sync { let sameUpdate = try! db.update(withId: testUpdate.updateId, config: config) @@ -114,3 +114,34 @@ class AppLauncherWithDatabaseSpec : ExpoSpec { } } } + +/// Allows for synchronization pertaining to the file scope. +private final class Synchronized { + private var _storage: T + private var lock = NSLock() + + /// Thread safe access here. + var value: T { + get { + return lockAround { + _storage + } + } + set { + lockAround { + _storage = newValue + } + } + } + + init(_ storage: T) { + self._storage = storage + } + + private func lockAround(_ closure: () -> U) -> U { + lock.lock() + defer { lock.unlock() } + return closure() + } + +} From 213472d134288156893a277c210fb1cfd3dda1a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Sun, 19 May 2024 12:53:27 +0200 Subject: [PATCH 5/8] [chore] add CHANGELOG + revert erroneously commit code --- .../Tests/EXDevLauncherURLHelperTests.swift | 4 ++-- packages/expo-modules-core/CHANGELOG.md | 2 +- .../ios/Tests/PersistentFileLogSpec.swift | 20 +++++++++---------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift b/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift index 53d49538c165d..a5fb14560ad51 100644 --- a/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift +++ b/packages/expo-dev-launcher/ios/Tests/EXDevLauncherURLHelperTests.swift @@ -24,8 +24,8 @@ class EXDevLauncherURLHelperTests: XCTestCase { XCTAssertEqual(URL(string: "http://expo-development-client/?url=http%3A%2F%2Flocalhost%3A8081"), actual2) // should not crash if provided URL does not include scheme -// let actual3 = EXDevLauncherURLHelper.replaceEXPScheme(URL(string: "192.168.0.12:8081")!, to: "scheme") -// XCTAssertEqual(URL(string: "192.168.0.12:8081"), actual3) + let actual3 = EXDevLauncherURLHelper.replaceEXPScheme(URL(string: "192.168.0.12:8081")!, to: "scheme") + XCTAssertEqual(URL(string: "192.168.0.12:8081"), actual3) } func testDevLauncherUrls() { diff --git a/packages/expo-modules-core/CHANGELOG.md b/packages/expo-modules-core/CHANGELOG.md index 9a9c34b1bb2d3..540c824d3e870 100644 --- a/packages/expo-modules-core/CHANGELOG.md +++ b/packages/expo-modules-core/CHANGELOG.md @@ -8,7 +8,7 @@ ### 🐛 Bug fixes -- Fix data race in `PersistentFileLogSpec.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) +- Fix data race in `PersistentFileLogSpec.swift` and `AppLauncherWithDatabaseMock.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) ### 💡 Others diff --git a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift index fa88eb52439f6..01a7b227a5671 100644 --- a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift +++ b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift @@ -48,7 +48,7 @@ class PersistentFileLogSpec: ExpoSpec { } static func clearEntriesSync(log: PersistentFileLog) { - let didClear = SynchronizedBool(false) + let didClear = Synchronized(false) log.clearEntries { _ in didClear.value = true } @@ -56,7 +56,7 @@ class PersistentFileLogSpec: ExpoSpec { } static func filterEntriesSync(log: PersistentFileLog, filter: @escaping PersistentFileLogFilter) { - let didPurge = SynchronizedBool(false) + let didPurge = Synchronized(false) log.purgeEntriesNotMatchingFilter(filter: filter) { _ in didPurge.value = true } @@ -64,7 +64,7 @@ class PersistentFileLogSpec: ExpoSpec { } static func appendEntrySync(log: PersistentFileLog, entry: String) { - let didAppend = SynchronizedBool(false) + let didAppend = Synchronized(false) log.appendEntry(entry: entry) { _ in didAppend.value = true } @@ -72,15 +72,13 @@ class PersistentFileLogSpec: ExpoSpec { } } -/// Since the boolean value is written to in the closure supplied to `log.appendEntry`, -/// while concurrenlty being read in `expect`, i.e., via the autoclosure, synchronization -/// is needed to avoid data races. This class allows for simple synchronization of a boolean value. -private final class SynchronizedBool { - private var _storage: Bool +/// Allows for synchronization pertaining to the file scope. +private final class Synchronized { + private var _storage: T private var lock = NSLock() /// Thread safe access here. - var value: Bool { + var value: T { get { return lockAround { _storage @@ -93,11 +91,11 @@ private final class SynchronizedBool { } } - init(_ storage: Bool) { + init(_ storage: T) { self._storage = storage } - private func lockAround(_ closure: () -> T) -> T { + private func lockAround(_ closure: () -> U) -> U { lock.lock() defer { lock.unlock() } return closure() From 9c7dc43c53bfa8d21aff49c4d0188221cd951b42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Sun, 19 May 2024 23:37:32 +0200 Subject: [PATCH 6/8] [fix] add correct files to changelogs --- packages/expo-modules-core/CHANGELOG.md | 2 +- packages/expo-updates/CHANGELOG.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/expo-modules-core/CHANGELOG.md b/packages/expo-modules-core/CHANGELOG.md index 540c824d3e870..9a9c34b1bb2d3 100644 --- a/packages/expo-modules-core/CHANGELOG.md +++ b/packages/expo-modules-core/CHANGELOG.md @@ -8,7 +8,7 @@ ### 🐛 Bug fixes -- Fix data race in `PersistentFileLogSpec.swift` and `AppLauncherWithDatabaseMock.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) +- Fix data race in `PersistentFileLogSpec.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) ### 💡 Others diff --git a/packages/expo-updates/CHANGELOG.md b/packages/expo-updates/CHANGELOG.md index 741baa0a52ed4..50b86ed18a719 100644 --- a/packages/expo-updates/CHANGELOG.md +++ b/packages/expo-updates/CHANGELOG.md @@ -8,6 +8,8 @@ ### 🐛 Bug fixes +- Fix data race in `AppLauncherWithDatabaseMock.swift`. ([#28924](https://github.com/expo/expo/pull/28924) by [@hakonk](https://github.com/hakonk)) + ### 💡 Others ## 0.25.13 — 2024-05-15 From 1b30fdee83f32d99ece6ddbc3d1209e34f66857e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Sun, 19 May 2024 23:38:07 +0200 Subject: [PATCH 7/8] Improve wait for result in AppLauncherWithDatabaseSpec --- .../ios/Tests/AppLauncherWithDatabaseSpec.swift | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift index f95a388c7d589..fd50db5643ea9 100644 --- a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift +++ b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift @@ -98,12 +98,7 @@ class AppLauncherWithDatabaseSpec : ExpoSpec { launcher.launchUpdate(withSelectionPolicy: SelectionPolicyFactory.filterAwarePolicy(withRuntimeVersion: "1")) { error, success in successValue.value = success } - - while successValue.value == nil { - Thread.sleep(forTimeInterval: 0.1) - } - - expect(successValue.value) == true + expect(successValue.value).toEventually(beTrue(), timeout: .milliseconds(10_000)) db.databaseQueue.sync { let sameUpdate = try! db.update(withId: testUpdate.updateId, config: config) From 447698bfbf3a45dba39afeaabf17c1866fcaf249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Kosmaty?= Date: Fri, 7 Jun 2024 10:54:00 +0200 Subject: [PATCH 8/8] [skip ci] Apply suggestions from code review Co-authored-by: Will Schurman --- .../expo-modules-core/ios/Tests/PersistentFileLogSpec.swift | 2 +- .../expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift index 01a7b227a5671..5e83e88c645c0 100644 --- a/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift +++ b/packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift @@ -75,7 +75,7 @@ class PersistentFileLogSpec: ExpoSpec { /// Allows for synchronization pertaining to the file scope. private final class Synchronized { private var _storage: T - private var lock = NSLock() + private let lock = NSLock() /// Thread safe access here. var value: T { diff --git a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift index fd50db5643ea9..05421fc196482 100644 --- a/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift +++ b/packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift @@ -113,7 +113,7 @@ class AppLauncherWithDatabaseSpec : ExpoSpec { /// Allows for synchronization pertaining to the file scope. private final class Synchronized { private var _storage: T - private var lock = NSLock() + private let lock = NSLock() /// Thread safe access here. var value: T {