Fix crash in @cached property value when computeValue was called from mutate(_:) (#3174)

Author: laevandus

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Add `reason` parameter to `flagMessage()` [#3173](https://github.com/GetStream/stream-chat-swift/issues/3173)
 ### 🐞 Fixed
 - Reset managed object contexts before removing all the data on logout [#3170](https://github.com/GetStream/stream-chat-swift/pull/3170)
+- Rare crash when `@Cached` property triggered fetching a new value with `computeValue` closure [#3174](https://github.com/GetStream/stream-chat-swift/pull/3174)
 ### 🔄 Changed
 - Replace message update request to partial update on message pinning [#3166](https://github.com/GetStream/stream-chat-swift/pull/3166)
 

Sources/StreamChat/Utils/Cached.swift

@@ -13,23 +13,21 @@ class Cached<T> {
     /// When the cached value is reset, this closure is used to lazily get a new value which is then cached again.
     @Atomic var computeValue: (() -> T)!
 
+    /// Returns the cached value if it is set, otherwise uses the `computeValue` closure to refetch the value.
+    ///
+    /// Since `computeValue` can trigger side-effects like CoreData to save a context which triggers database
+    /// observers then it is important to run the `computeValue` closure outside of the locked region (e.g. `_cached.mutate(_:).
+    /// Otherwise we might reenter the same instance while the `_cached` lock has not released the lock yet.
+    /// Only downside is that we might need to execute `computeValue` multiple times if there are multiple threads
+    /// accessing the `wrappedValue` at the same time while `_cached` is nil.
     var wrappedValue: T {
-        var returnValue: T!
-
-        // We need to make the changes inside the `mutate` block to ensure `Cached` is thread-safe.
-        __cached.mutate { value in
-            if let value = value {
-                returnValue = value
-                return
-            }
-
-            log.assert(computeValue != nil, "You must set the `computeValue` closure before accessing the cached value.")
-
-            value = computeValue()
-            returnValue = value
+        if let _cached {
+            return _cached
         }
-
-        return returnValue
+        log.assert(computeValue != nil, "You must set the `computeValue` closure before accessing the cached value.")
+        let newValue = computeValue()
+        _cached = newValue
+        return newValue
     }
 
     var projectedValue: (() -> T) {

Tests/StreamChatTests/StreamChatStressTests/Cached_StressTests.swift

@@ -7,6 +7,7 @@ import StreamChatTestTools
 import XCTest
 
 final class Cached_Tests: StressTestCase {
+    let queue = DispatchQueue(label: "io.getstream.tests.cached")
     var counter: Int!
     @Cached var value: Int!
 
@@ -18,8 +19,11 @@ final class Cached_Tests: StressTestCase {
 
         _value.computeValue = { [unowned self] in
             // Return the current counter value and increase the counter
-            defer { self.counter += 1 }
-            return self.counter
+            queue.sync {
+                let value = counter
+                counter += 1
+                return value
+            }
         }
     }
 
@@ -65,4 +69,49 @@ final class Cached_Tests: StressTestCase {
         }
         group.wait()
     }
+    
+    func test_resetAndRead_whenRunningConcurrently() {
+        _value.computeValue = { Int.max }
+        DispatchQueue.concurrentPerform(iterations: 10) { _ in
+            self._value.reset()
+            _ = self.value
+        }
+        XCTAssertEqual(value, Int.max)
+    }
+    
+    func test_exclusiveAccess_whenComputeValueTriggersReset() throws {
+        // 1. `EntityDatabaseObserver.item` was accessed but since the cached value was nil, `computeValue` was called
+        // 2. Compute value for the `EntityDatabaseObserver.item` triggered CoreData object change
+        // 3. CoreData change was picked up by the `EntityDatabaseObserver` which ended up calling `item.reset()`
+        // Result: Crash because computeValue was called from the Atomic's mutate function and therefore all the code was
+        // triggered from the mutate. `item.reset()` triggered another mutate and then Swift's runtime crashed the app
+        // because it requires exclusive access (`mutate` uses inout parameters!).
+        // This test would crash if `computeValue` is called within `mutate`.
+        let loader = ExclusiveAccessTriggeringLoader(computeValueResult: 1)
+        let result = loader.readValue()
+        XCTAssertEqual(result, 1)
+    }
+}
+
+private extension Cached_Tests {
+    /// Triggers crash if `computeValue` is called from `mutate(_:)`.
+    final class ExclusiveAccessTriggeringLoader {
+        @Cached private var value: Int?
+
+        init(computeValueResult: Int) {
+            _value.computeValue = { [weak self] in
+                self?.simulateComputeValueTriggeringCoreDataChangeWhichLeadsToCallingResetOnTheSameProperty()
+                return computeValueResult
+            }
+        }
+
+        func readValue() -> Int? {
+            value
+        }
+
+        private func simulateComputeValueTriggeringCoreDataChangeWhichLeadsToCallingResetOnTheSameProperty() {
+            // Simulates the case of a CoreData change which in turn ended up calling reset on the item
+            _value.reset()
+        }
+    }
 }