Fix caching of dependencies during re-entrancy (#249)

* Fix cache. * wip; * wip * simplify test * simplify * fix * Clear cache at end of test so to not bleed over into other tests. * fix test * wip
pointfreeco · Jul 22, 2024 · d5798b5 · d5798b5
1 parent d806136
commit d5798b5
Show file tree

Hide file tree

Showing 2 changed files with 118 additions and 70 deletions.
diff --git a/Sources/Dependencies/DependencyValues.swift b/Sources/Dependencies/DependencyValues.swift
@@ -119,7 +119,8 @@ public struct DependencyValues: Sendable {
   @TaskLocal static var isSetting = false
   @TaskLocal static var currentDependency = CurrentDependency()
 
-  fileprivate var cachedValues = CachedValues()
+  @_spi(Internals)
+  public var cachedValues = CachedValues()
   private var storage: [ObjectIdentifier: any Sendable] = [:]
 
   /// Creates a dependency values instance.
@@ -140,7 +141,7 @@ public struct DependencyValues: Sendable {
             .takeUnretainedValue()
         else { return }
         let testCaseWillStartBlock: @convention(block) (AnyObject) -> Void = { _ in
-          DependencyValues._current.cachedValues.cached.withValue { $0 = [:] }
+          DependencyValues._current.cachedValues.cached = [:]
         }
         let testCaseWillStartImp = imp_implementationWithBlock(testCaseWillStartBlock)
         class_addMethod(
@@ -154,7 +155,7 @@ public struct DependencyValues: Sendable {
       if _XCTIsTesting {
         XCTestObservationCenter.shared.addTestObserver(
           TestObserver({
-            DependencyValues._current.cachedValues.cached.withValue { $0 = [:] }
+            DependencyValues._current.cachedValues.cached = [:]
           }))
       }
     #else
@@ -177,7 +178,7 @@ public struct DependencyValues: Sendable {
         }
       #endif
       pRegisterTestObserver?({
-        DependencyValues._current.cachedValues.cached.withValue { $0 = [:] }
+        DependencyValues._current.cachedValues.cached = [:]
       })
     #endif
   }
@@ -336,13 +337,15 @@ private let defaultContext: DependencyContext = {
   }
 }()
 
-private final class CachedValues: Sendable {
-  struct CacheKey: Hashable, Sendable {
+@_spi(Internals)
+public final class CachedValues: @unchecked Sendable {
+  public struct CacheKey: Hashable, Sendable {
     let id: ObjectIdentifier
     let context: DependencyContext
   }
 
-  fileprivate let cached = LockIsolated([CacheKey: any Sendable]())
+  private let lock = NSRecursiveLock()
+  public var cached = [CacheKey: any Sendable]()
 
   func value<Key: TestDependencyKey>(
     for key: Key.Type,
@@ -351,90 +354,91 @@ private final class CachedValues: Sendable {
     function: StaticString = #function,
     line: UInt = #line
   ) -> Key.Value {
-    XCTFailContext.$current.withValue(XCTFailContext(file: file, line: line)) {
-      self.cached.withValue { cached in
-        let cacheKey = CacheKey(id: ObjectIdentifier(key), context: context)
-        guard let base = cached[cacheKey], let value = base as? Key.Value
-        else {
-          let value: Key.Value?
-          switch context {
-          case .live:
-            value = (key as? any DependencyKey.Type)?.liveValue as? Key.Value
-          case .preview:
-            value = Key.previewValue
-          case .test:
-            value = Key.testValue
-          }
+    lock.lock()
+    defer { lock.unlock() }
+
+    return XCTFailContext.$current.withValue(XCTFailContext(file: file, line: line)) {
+      let cacheKey = CacheKey(id: ObjectIdentifier(key), context: context)
+      guard let base = cached[cacheKey], let value = base as? Key.Value
+      else {
+        let value: Key.Value?
+        switch context {
+        case .live:
+          value = (key as? any DependencyKey.Type)?.liveValue as? Key.Value
+        case .preview:
+          value = Key.previewValue
+        case .test:
+          value = Key.testValue
+        }
 
-          guard let value
-          else {
-            #if DEBUG
-              if !DependencyValues.isSetting {
-                var dependencyDescription = ""
-                if let fileID = DependencyValues.currentDependency.fileID,
-                  let line = DependencyValues.currentDependency.line
-                {
-                  dependencyDescription.append(
+        guard let value
+        else {
+          #if DEBUG
+          if !DependencyValues.isSetting {
+            var dependencyDescription = ""
+            if let fileID = DependencyValues.currentDependency.fileID,
+               let line = DependencyValues.currentDependency.line
+            {
+              dependencyDescription.append(
                     """
                       Location:
                         \(fileID):\(line)
 
                     """
-                  )
-                }
-                dependencyDescription.append(
-                  Key.self == Key.Value.self
-                    ? """
+              )
+            }
+            dependencyDescription.append(
+              Key.self == Key.Value.self
+              ? """
                       Dependency:
                         \(typeName(Key.Value.self))
                     """
-                    : """
+              : """
                       Key:
                         \(typeName(Key.self))
                       Value:
                         \(typeName(Key.Value.self))
                     """
-                )
-
-                var argument: String {
-                  "\(function)" == "subscript(_:)" ? "\(typeName(Key.self)).self" : "\\.\(function)"
-                }
-
-                runtimeWarn(
-                  """
-                  @Dependency(\(argument)) has no live implementation, but was accessed from a live \
-                  context.
-
-                  \(dependencyDescription)
-
-                  To fix you can do one of two things:
-
-                  * Conform '\(typeName(Key.self))' to the 'DependencyKey' protocol by providing \
-                  a live implementation of your dependency, and make sure that the conformance is \
-                  linked with this current application.
-
-                  * Override the implementation of '\(typeName(Key.self))' using 'withDependencies'. \
-                  This is typically done at the entry point of your application, but can be done \
-                  later too.
-                  """,
-                  file: DependencyValues.currentDependency.file ?? file,
-                  line: DependencyValues.currentDependency.line ?? line
-                )
-              }
-            #endif
-            let value = Key.testValue
-            if !DependencyValues.isSetting {
-              cached[cacheKey] = value
+            )
+
+            var argument: String {
+              "\(function)" == "subscript(_:)" ? "\(typeName(Key.self)).self" : "\\.\(function)"
             }
-            return value
-          }
 
-          cached[cacheKey] = value
+            runtimeWarn(
+              """
+              @Dependency(\(argument)) has no live implementation, but was accessed from a live \
+              context.
+
+              \(dependencyDescription)
+
+              To fix you can do one of two things:
+
+              * Conform '\(typeName(Key.self))' to the 'DependencyKey' protocol by providing \
+              a live implementation of your dependency, and make sure that the conformance is \
+              linked with this current application.
+
+              * Override the implementation of '\(typeName(Key.self))' using 'withDependencies'. \
+              This is typically done at the entry point of your application, but can be done \
+              later too.
+              """,
+              file: DependencyValues.currentDependency.file ?? file,
+              line: DependencyValues.currentDependency.line ?? line
+            )
+          }
+          #endif
+          let value = Key.testValue
+          if !DependencyValues.isSetting {
+            cached[cacheKey] = value
+          }
           return value
         }
 
+        cached[cacheKey] = value
         return value
       }
+
+      return value
     }
   }
 }
diff --git a/Tests/DependenciesTests/CacheTests.swift b/Tests/DependenciesTests/CacheTests.swift
@@ -0,0 +1,44 @@
+@_spi(Internals) import Dependencies
+import XCTest
+
+final class CachedValueTests: XCTestCase {
+  override func tearDown() {
+    super.tearDown()
+    CacheLocals.$skipFailure.withValue(true) {
+      DependencyValues._current.cachedValues.cached = [:]
+    }
+  }
+
+  func testCacheWithReEntrantAccess() {
+    @Dependency(OuterDependencyTests.self) var outerDependency
+    _ = outerDependency
+  }
+}
+
+private struct OuterDependencyTests: TestDependencyKey {
+  static var testValue: OuterDependencyTests {
+    @Dependency(InnerDependency.self) var innerDependency
+    _ = innerDependency
+    return Self()
+  }
+}
+private struct InnerDependency: TestDependencyKey {
+  let perform: @Sendable () -> Void
+  static var testValue: InnerDependency {
+    final class Ref: Sendable {
+      deinit {
+        guard !CacheLocals.skipFailure
+        else { return }
+        XCTFail("This should not deinit")
+      }
+    }
+    let ref = Ref()
+    return Self {
+      _ = ref
+    }
+  }
+}
+
+private enum CacheLocals {
+  @TaskLocal static var skipFailure = false
+}