Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making the ChangeTime object thread safe #46

Merged
merged 1 commit into from
Oct 27, 2016
Merged

Making the ChangeTime object thread safe #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions RocketData.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
611562DB1CC16B490001F5CE /* CollectionChangeTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 611562DA1CC16B490001F5CE /* CollectionChangeTests.swift */; };
6117DDF81CD7A8EB002F57C1 /* BatchDataProviderListener.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDE51CD7A8EB002F57C1 /* BatchDataProviderListener.swift */; };
6117DDFA1CD7A8EB002F57C1 /* CacheDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDE71CD7A8EB002F57C1 /* CacheDelegate.swift */; };
6117DDFB1CD7A8EB002F57C1 /* ChangeClock.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDE81CD7A8EB002F57C1 /* ChangeClock.swift */; };
6117DDFB1CD7A8EB002F57C1 /* ChangeTime.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDE81CD7A8EB002F57C1 /* ChangeTime.swift */; };
6117DDFC1CD7A8EB002F57C1 /* CollectionChange.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDE91CD7A8EB002F57C1 /* CollectionChange.swift */; };
6117DDFD1CD7A8EB002F57C1 /* CollectionDataProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDEA1CD7A8EB002F57C1 /* CollectionDataProvider.swift */; };
6117DDFE1CD7A8EB002F57C1 /* ConsistencyContextWrapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6117DDEB1CD7A8EB002F57C1 /* ConsistencyContextWrapper.swift */; };
Expand Down Expand Up @@ -66,7 +66,7 @@
611562DA1CC16B490001F5CE /* CollectionChangeTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CollectionChangeTests.swift; sourceTree = "<group>"; };
6117DDE51CD7A8EB002F57C1 /* BatchDataProviderListener.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BatchDataProviderListener.swift; sourceTree = "<group>"; };
6117DDE71CD7A8EB002F57C1 /* CacheDelegate.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CacheDelegate.swift; sourceTree = "<group>"; };
6117DDE81CD7A8EB002F57C1 /* ChangeClock.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ChangeClock.swift; sourceTree = "<group>"; };
6117DDE81CD7A8EB002F57C1 /* ChangeTime.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ChangeTime.swift; sourceTree = "<group>"; };
6117DDE91CD7A8EB002F57C1 /* CollectionChange.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CollectionChange.swift; sourceTree = "<group>"; };
6117DDEA1CD7A8EB002F57C1 /* CollectionDataProvider.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CollectionDataProvider.swift; sourceTree = "<group>"; };
6117DDEB1CD7A8EB002F57C1 /* ConsistencyContextWrapper.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ConsistencyContextWrapper.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -147,7 +147,7 @@
6117DDF41CD7A8EB002F57C1 /* ParsingHelpers.swift */,
6117DDF01CD7A8EB002F57C1 /* Errors.swift */,
6117DDF21CD7A8EB002F57C1 /* Logger.swift */,
6117DDE81CD7A8EB002F57C1 /* ChangeClock.swift */,
6117DDE81CD7A8EB002F57C1 /* ChangeTime.swift */,
6117DDEB1CD7A8EB002F57C1 /* ConsistencyContextWrapper.swift */,
6117DDED1CD7A8EB002F57C1 /* DataHolder.swift */,
6117DDF61CD7A8EB002F57C1 /* SharedCollectionManager.swift */,
Expand Down Expand Up @@ -458,7 +458,7 @@
6117DE041CD7A8EB002F57C1 /* Logger.swift in Sources */,
6117DDF81CD7A8EB002F57C1 /* BatchDataProviderListener.swift in Sources */,
6117DE051CD7A8EB002F57C1 /* Model.swift in Sources */,
6117DDFB1CD7A8EB002F57C1 /* ChangeClock.swift in Sources */,
6117DDFB1CD7A8EB002F57C1 /* ChangeTime.swift in Sources */,
6117DDFD1CD7A8EB002F57C1 /* CollectionDataProvider.swift in Sources */,
6117DDFA1CD7A8EB002F57C1 /* CacheDelegate.swift in Sources */,
6117DE091CD7A8EB002F57C1 /* WeakSharedCollectionArray.swift in Sources */,
Expand Down
14 changes: 10 additions & 4 deletions RocketData/ChangeClock.swift → RocketData/ChangeTime.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,27 @@ import Foundation
/**
This class keeps a global clock which is used to record when changes happen.
Whenever you create a new ChangeTime, it will be after any previous times and before any future times.
It is not thread-safe. You must call it on the main thread.
This is a thread safe class. You can call it from any thread.
*/
struct ChangeTime: Equatable {
/// Keeps track of the last time we updated
private static var lastTime = 1
/// We use this serial queue to sync on different threads
private static let queue = DispatchQueue(label: "com.rocketdata.changeTime")

fileprivate let time: Int

/**
Creates a new ChangeTime instance. This is guarenteed to be after any previous times created.
*/
init() {
Log.sharedInstance.assert(Thread.isMainThread, "The ChangeClock was accessed on a different thread than the main thread. This probably means you are accessing something in the library that is not thread-safe on a different thread. This can cause race conditions.")
self.time = ChangeTime.lastTime
ChangeTime.lastTime += 1
var time = 0
ChangeTime.queue.sync {
time = ChangeTime.lastTime
ChangeTime.lastTime += 1
}
Log.sharedInstance.assert(time != 0, "Dispatch sync is behaving incorrectly. This is a bug.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a unit test for ChangeTime would be better.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, new tests fail. I think I still want this assert to be doubly sure. I can also add a unit test for change time (why not?)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I checked. Already have a unit test for change time. Verifies that if you call it twice in a row, its incrementing and > zero.

self.time = time
}

/**
Expand Down
78 changes: 78 additions & 0 deletions RocketDataTests/DataModelManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import XCTest
import RocketData

class DataModelManagerTests: RocketDataTestCase {

// MARK: Update Model

func testUpdateModelWithCache() {
let cacheExpectation = expectation(description: "Wait for cache")
Expand Down Expand Up @@ -67,6 +69,41 @@ class DataModelManagerTests: RocketDataTestCase {
XCTAssertEqual(dataProvider[0], newModel)
}

func testUpdateModelDifferentThread() {
let cacheExpectation = expectation(description: "Wait for cache")
let cache = ExpectCacheDelegate()
let dataModelManager = DataModelManager(cacheDelegate: cache)
let dataProvider = CollectionDataProvider<ParentModel>(dataModelManager: dataModelManager)

let initialModel = ParentModel(id: 1, name: "initial", requiredChild: ChildModel(), otherChildren: [])
let newModel = ParentModel(id: 1, name: "new", requiredChild: ChildModel(), otherChildren: [])

let delegate = ClosureCollectionDataProviderDelegate() { (collectionChanges, context) in
XCTAssertEqual(context as? String, "context")
}
dataProvider.delegate = delegate

cache.setModelCalled = { model, key, context in
XCTAssertEqual(model as? ParentModel, newModel)
XCTAssertEqual(context as? String, "context")
XCTAssertEqual(key, "ParentModel:1")
cacheExpectation.fulfill()
}

dataProvider.setData([initialModel], cacheKey: nil, context: "wrong")

DispatchQueue.global(qos: .default).async {
dataModelManager.updateModel(newModel, context: "context")
}

waitForExpectations(timeout: 10, handler: nil)
waitForConsistencyManagerToFlush(dataModelManager.consistencyManager)

XCTAssertEqual(dataProvider[0], newModel)
}

// MARK: Update Models

func testUpdateModelsWithCache() {
let cacheExpectation = expectation(description: "Wait for cache")
let cache = ExpectCacheDelegate()
Expand Down Expand Up @@ -133,6 +170,47 @@ class DataModelManagerTests: RocketDataTestCase {
XCTAssertEqual(dataProvider[1], otherNewModel)
}

func testUpdateModelsDifferentThread() {
let cacheExpectation = expectation(description: "Wait for cache")
let cache = ExpectCacheDelegate()
let dataModelManager = DataModelManager(cacheDelegate: cache)
let dataProvider = CollectionDataProvider<ParentModel>(dataModelManager: dataModelManager)

let initialModel = ParentModel(id: 1, name: "initial", requiredChild: ChildModel(), otherChildren: [])
let otherInitialModel = ParentModel(id: 2, name: "initial", requiredChild: ChildModel(), otherChildren: [])
let newModel = ParentModel(id: 1, name: "new", requiredChild: ChildModel(), otherChildren: [])
let otherNewModel = ParentModel(id: 2, name: "new", requiredChild: ChildModel(), otherChildren: [])

let delegate = ClosureCollectionDataProviderDelegate() { (collectionChanges, context) in
XCTAssertEqual(context as? String, "context")
}
dataProvider.delegate = delegate

var setModelCalled = 0
cache.setModelCalled = { model, key, context in
setModelCalled += 1
XCTAssertEqual(model as? ParentModel, setModelCalled == 1 ? newModel : otherNewModel)
XCTAssertEqual(context as? String, "context")
XCTAssertEqual(key, "ParentModel:\(setModelCalled)")
if setModelCalled == 2 {
cacheExpectation.fulfill()
}
}

dataProvider.setData([initialModel, otherInitialModel], cacheKey: nil, context: "wrong")
DispatchQueue.global(qos: .default).async {
dataModelManager.updateModels([newModel, otherNewModel], context: "context")
}

waitForExpectations(timeout: 10, handler: nil)
waitForConsistencyManagerToFlush(dataModelManager.consistencyManager)

XCTAssertEqual(dataProvider[0], newModel)
XCTAssertEqual(dataProvider[1], otherNewModel)
}

// MARK: Compilation Tests

/**
Previously, this test failed because of https://bugs.swift.org/browse/SR-3038
This test passes if it compiles.
Expand Down