Memory leak fix & more sanitization in tests

.travis.yml

@@ -83,7 +83,6 @@ script: |
   echo —————— Running tests for $platform ...
   echo
 
-  Siesta_DelayAfterEachSpec=1 \
   Siesta_TestMultipleNetworkProviders=1 \
   travis_retry xcodebuild test \
       -project Siesta.xcodeproj \

Cartfile.private

@@ -1,10 +1,10 @@
 # Optional integrations
 
-github "Alamofire/Alamofire" ~> 4.0
+github "Alamofire/Alamofire"
 # github "ReactiveCocoa/ReactiveCocoa" "master"  # add to Cartfile if/when it has tests
 
 # Testing
 
-github "Quick/Quick"
+github "pcantrell/Quick" "around-each"
 github "Quick/Nimble"
 github "pcantrell/Nocilla" "siesta"                     # fork adds delay() / go(), nullability annotations

Cartfile.resolved

@@ -1,4 +1,4 @@
-github "Alamofire/Alamofire" "4.7.2"
-github "Quick/Nimble" "v7.1.2"
-github "Quick/Quick" "v1.3.0"
+github "Alamofire/Alamofire" "4.7.3"
+github "Quick/Nimble" "v7.1.3"
 github "pcantrell/Nocilla" "bd7ec7caa0576f08c00bbbf993a9204f93be16e3"
+github "pcantrell/Quick" "eeaddb112fc486b1e3699a5985e6a68dd5bc56d8"

Siesta.xcodeproj/xcshareddata/xcschemes/Siesta iOS.xcscheme

@@ -40,6 +40,8 @@
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      enableThreadSanitizer = "YES"
+      enableUBSanitizer = "YES"
       codeCoverageEnabled = "YES"
       shouldUseLaunchSchemeArgsEnv = "NO">
       <Testables>
@@ -64,18 +66,18 @@
          </BuildableReference>
       </MacroExpansion>
       <EnvironmentVariables>
-         <EnvironmentVariable
-            key = "Siesta_DelayAfterEachSpec"
-            value = "$(Siesta_DelayAfterEachSpec)"
-            isEnabled = "YES">
-         </EnvironmentVariable>
          <EnvironmentVariable
             key = "Siesta_TestMultipleNetworkProviders"
             value = "$(Siesta_TestMultipleNetworkProviders)"
             isEnabled = "YES">
          </EnvironmentVariable>
       </EnvironmentVariables>
       <AdditionalOptions>
+         <AdditionalOption
+            key = "NSZombieEnabled"
+            value = "YES"
+            isEnabled = "YES">
+         </AdditionalOption>
       </AdditionalOptions>
    </TestAction>
    <LaunchAction

Siesta.xcodeproj/xcshareddata/xcschemes/Siesta macOS.xcscheme

@@ -40,6 +40,9 @@
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      enableThreadSanitizer = "YES"
+      enableUBSanitizer = "YES"
+      codeCoverageEnabled = "YES"
       shouldUseLaunchSchemeArgsEnv = "NO">
       <Testables>
          <TestableReference
@@ -63,28 +66,33 @@
          </BuildableReference>
       </MacroExpansion>
       <EnvironmentVariables>
-         <EnvironmentVariable
-            key = "Siesta_DelayAfterEachSpec"
-            value = "$(Siesta_DelayAfterEachSpec)"
-            isEnabled = "YES">
-         </EnvironmentVariable>
          <EnvironmentVariable
             key = "Siesta_TestMultipleNetworkProviders"
             value = "$(Siesta_TestMultipleNetworkProviders)"
             isEnabled = "YES">
          </EnvironmentVariable>
       </EnvironmentVariables>
       <AdditionalOptions>
+         <AdditionalOption
+            key = "NSZombieEnabled"
+            value = "YES"
+            isEnabled = "YES">
+         </AdditionalOption>
       </AdditionalOptions>
    </TestAction>
    <LaunchAction
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      enableASanStackUseAfterReturn = "YES"
+      disableMainThreadChecker = "YES"
       launchStyle = "0"
       useCustomWorkingDirectory = "NO"
       ignoresPersistentStateOnLaunch = "NO"
       debugDocumentVersioning = "YES"
+      stopOnEveryThreadSanitizerIssue = "YES"
+      stopOnEveryUBSanitizerIssue = "YES"
+      stopOnEveryMainThreadCheckerIssue = "YES"
       debugServiceExtension = "internal"
       allowLocationSimulation = "YES">
       <MacroExpansion>

Siesta.xcodeproj/xcshareddata/xcschemes/Siesta tvOS.xcscheme

@@ -26,10 +26,16 @@
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      enableThreadSanitizer = "YES"
       shouldUseLaunchSchemeArgsEnv = "YES">
       <Testables>
       </Testables>
       <AdditionalOptions>
+         <AdditionalOption
+            key = "NSZombieEnabled"
+            value = "YES"
+            isEnabled = "YES">
+         </AdditionalOption>
       </AdditionalOptions>
    </TestAction>
    <LaunchAction

Siesta.xcodeproj/xcshareddata/xcschemes/Siesta watchOS.xcscheme

@@ -30,6 +30,11 @@
       <Testables>
       </Testables>
       <AdditionalOptions>
+         <AdditionalOption
+            key = "NSZombieEnabled"
+            value = "YES"
+            isEnabled = "YES">
+         </AdditionalOption>
       </AdditionalOptions>
    </TestAction>
    <LaunchAction

Source/Siesta/Resource/Resource.swift

@@ -9,8 +9,7 @@ import Foundation
 
 
 // Overridable for testing
-internal var fakeNow: TimeInterval?
-internal let now = { fakeNow ?? Date.timeIntervalSinceReferenceDate }
+internal var now = { Date.timeIntervalSinceReferenceDate }
 
 /**
   An in-memory cache of a RESTful resource, plus information about the status of network requests related to it.
@@ -376,14 +375,28 @@ public final class Resource: NSObject
 
         if case .inProgress(let cacheRequest) = cacheCheckStatus
             {
+            // isLoading needs to be:
+            //  - false at first,
+            //  - true after loadIfNeeded() while the cache check is in progress, but
+            //  - false again before observers receive a cache hit.
+            //
+            // To make this happen, we need to add the chained cacheThenNetwork below
+            // to loadRequests after it’s created, but remove it _before_ the request
+            // chain proceeds. The trickery with trackedRequest below makes this happen.
+
             var trackedRequest: Request?
+            cacheRequest.onCompletion
+                {
+                _ in self.loadRequests.remove { $0 === trackedRequest }
+                }
+
+            // Now we're ready to construct a chained request that will return either a
+            // cache hit or a bona fide network result.
+
             let cacheThenNetwork = cacheRequest.chained
                 {
                 _ in // We don’t need the result of the cache request here; resource state is already updated
 
-                // Ensure isLoading is false for last event observers receive
-                self.loadRequests.remove { $0 === trackedRequest }
-
                 if self.isUpToDate                 // If cached data is up to date...
                     {
                     self.receiveDataNotModified()  // ...tell observers isLoading is false...
@@ -394,8 +407,10 @@ public final class Resource: NSObject
                     return .passTo(self.load())    // Cache was a bust, so make the real request
                     }
                 }
+
             loadRequests.append(cacheThenNetwork)
             trackedRequest = cacheThenNetwork
+
             return cacheThenNetwork
             }
 

Source/Siesta/Resource/ResourceObserver.swift

@@ -420,5 +420,5 @@ private struct ClosureObserver: ResourceObserver, CustomDebugStringConvertible
 extension Resource: WeakCacheValue
     {
     func allowRemovalFromCache()
-        { cleanDefunctObservers() }
+        { cleanDefunctObservers(force: true) }
     }

Tests/Functional/EntityCacheSpec.swift

@@ -86,6 +86,7 @@ class EntityCacheSpec: ResourceSpecBase
                     if let firstRequest = requests.first!
                         { awaitNewData(firstRequest) }
                     expect(resource().isLoading).toEventually(beFalse())
+                    resource().removeObservers(ownedBy: eventRecorder())
                     awaitObserverCleanup(for: resource())
                     }
 
@@ -281,7 +282,8 @@ private class TestCache: EntityCache
         DispatchQueue.main.asyncAfter(deadline: .now() + 0.05)
             { self.receivedCacheRead = true }
 
-        return entries[key]
+        return DispatchQueue.main.sync
+            { entries[key] }
         }
 
     func writeEntity(_ entity: Entity<Any>, forKey key: TestCacheKey)
@@ -294,7 +296,10 @@ private class TestCache: EntityCache
         }
 
     func removeEntity(forKey key: TestCacheKey)
-        { entries.removeValue(forKey: key) }
+        {
+        _ = DispatchQueue.main.sync
+            { entries.removeValue(forKey: key) }
+        }
     }
 
 private struct TestCacheKey

Tests/Functional/ProgressSpec.swift

@@ -54,8 +54,10 @@ class ProgressSpec: ResourceSpecBase
                 let req = resource().load()
                 req.cancel()
                 expect(req.progress) == 1.0
-                _ = reqStub.go()
                 awaitFailure(req, initialState: .completed)
+
+                _ = reqStub.go()
+                awaitCancelledRequests()
                 }
             }
 

Tests/Functional/ResourceObserversSpec.swift

@@ -24,6 +24,10 @@ class ResourceObserversSpec: ResourceSpecBase
 
         afterEach
             {
+            observer.checkForUnfulfilledExpectations()
+            observer.stopTesting()
+            resource().removeObservers(ownedBy: observer)
+            simulateMemoryWarning()
             awaitObserverCleanup(for: resource())
             observer = nil
             }
@@ -189,6 +193,8 @@ class ResourceObserversSpec: ResourceSpecBase
                 req.cancel()
                 _ = reqStub.go()
                 awaitFailure(req, initialState: .completed)
+
+                awaitCancelledRequests()
                 }
 
             it("receives failure event")
@@ -227,6 +233,8 @@ class ResourceObserversSpec: ResourceSpecBase
                 awaitNewData(resource().load())
 
                 expect(events) == ["observerAdded", "requested", "newData(network)"]
+
+                resource().removeObservers(ownedBy: dummy)
                 }
 
             it("can have multiple closure observers")
@@ -250,6 +258,8 @@ class ResourceObserversSpec: ResourceSpecBase
 
                 expect(events0) == ["observerAdded", "requested", "newData(network)", "requested", "newData(network)"]
                 expect(events1) == ["observerAdded", "requested", "newData(network)"]
+
+                resource().removeObservers(ownedBy: dummy)
                 }
 
             it("is not added twice if it is an object")
@@ -263,15 +273,21 @@ class ResourceObserversSpec: ResourceSpecBase
                 awaitNewData(resource().load())
                 }
 
-            context("with multiple owners")
+            describe("with multiple owners")
                 {
-                let owner1 = specVar { NSData() },
-                    owner2 = specVar { NSString() }
+                let owner1 = NSData(),
+                    owner2 = NSString()
 
                 beforeEach
                     {
-                    resource().addObserver(observer, owner: owner1())
-                    resource().addObserver(observer, owner: owner2())
+                    resource().addObserver(observer, owner: owner1)
+                    resource().addObserver(observer, owner: owner2)
+                    }
+
+                afterEach
+                    {
+                    resource().removeObservers(ownedBy: owner1)
+                    resource().removeObservers(ownedBy: owner2)
                     }
 
                 func expectStillObserving(_ stillObserving: Bool)
@@ -288,25 +304,26 @@ class ResourceObserversSpec: ResourceSpecBase
                     awaitNewData(resource().load())
                     }
 
+
                 it("is not removed if self-ownership is not removed")
                     {
-                    resource().removeObservers(ownedBy: owner1())
-                    resource().removeObservers(ownedBy: owner2())
+                    resource().removeObservers(ownedBy: owner1)
+                    resource().removeObservers(ownedBy: owner2)
                     expectStillObserving(true)
                     }
 
                 it("is not removed if external owner is not removed")
                     {
                     resource().removeObservers(ownedBy: observer)
-                    resource().removeObservers(ownedBy: owner2())
+                    resource().removeObservers(ownedBy: owner2)
                     expectStillObserving(true)
                     }
 
                 it("is removed when all owners are removed")
                     {
                     resource().removeObservers(ownedBy: observer)
-                    resource().removeObservers(ownedBy: owner1())
-                    resource().removeObservers(ownedBy: owner2())
+                    resource().removeObservers(ownedBy: owner1)
+                    resource().removeObservers(ownedBy: owner2)
                     expectStillObserving(false)
                     }
                 }
@@ -458,10 +475,8 @@ private class TestObserver: ResourceObserver
 
 private class TestObserverWithExpectations: ResourceObserver
     {
-    private var expectedEvents = [Expectation]()
-
-    deinit
-        { checkForUnfulfilledExpectations() }
+    private var expectedEvents: [Expectation] = []
+    private var testing = true
 
     func expect(_ events: ResourceEvent..., callback: @escaping (() -> Void) = {})
         {
@@ -480,6 +495,11 @@ private class TestObserverWithExpectations: ResourceObserver
             { XCTFail("Expected observer events, but never received them: \(expectedEvents.map { $0.event })") }
         }
 
+    func stopTesting()
+        {
+        testing = false
+        }
+
     func resourceChanged(_ resource: Resource, event: ResourceEvent)
         {
         consume(event: "\(event)")
@@ -492,6 +512,8 @@ private class TestObserverWithExpectations: ResourceObserver
 
     private func consume(event: String)
         {
+        guard testing else { return }
+
         if expectedEvents.isEmpty
             { XCTFail("Received unexpected observer event: \(event)") }
         else