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.

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

Add thread safety #31

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 8 additions & 0 deletions Dip/Dip.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
0919F4EC1C16419500DC3B10 /* DefinitionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0919F4CF1C16417000DC3B10 /* DefinitionTests.swift */; };
0919F4ED1C16419500DC3B10 /* RuntimeArgumentsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0919F4D21C16417000DC3B10 /* RuntimeArgumentsTests.swift */; };
0919F4EE1C16419500DC3B10 /* ComponentScopeTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0919F4CE1C16417000DC3B10 /* ComponentScopeTests.swift */; };
2C15B9511C25F01200EA3486 /* ThreadSafetyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2C15B94F1C25EF7800EA3486 /* ThreadSafetyTests.swift */; };
2C15B9521C25F01300EA3486 /* ThreadSafetyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2C15B94F1C25EF7800EA3486 /* ThreadSafetyTests.swift */; };
2C15B9531C25F01500EA3486 /* ThreadSafetyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2C15B94F1C25EF7800EA3486 /* ThreadSafetyTests.swift */; };
/* End PBXBuildFile section */

/* Begin PBXContainerItemProxy section */
Expand Down Expand Up @@ -82,6 +85,7 @@
0919F4D01C16417000DC3B10 /* DipTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DipTests.swift; sourceTree = "<group>"; };
0919F4D11C16417000DC3B10 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
0919F4D21C16417000DC3B10 /* RuntimeArgumentsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RuntimeArgumentsTests.swift; sourceTree = "<group>"; };
2C15B94F1C25EF7800EA3486 /* ThreadSafetyTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ThreadSafetyTests.swift; sourceTree = "<group>"; };
/* End PBXFileReference section */

/* Begin PBXFrameworksBuildPhase section */
Expand Down Expand Up @@ -160,6 +164,7 @@
0919F4D21C16417000DC3B10 /* RuntimeArgumentsTests.swift */,
0919F4CE1C16417000DC3B10 /* ComponentScopeTests.swift */,
0919F4D11C16417000DC3B10 /* Info.plist */,
2C15B94F1C25EF7800EA3486 /* ThreadSafetyTests.swift */,
);
path = DipTests;
sourceTree = "<group>";
Expand Down Expand Up @@ -477,6 +482,7 @@
buildActionMask = 2147483647;
files = (
0919F4E61C16419300DC3B10 /* ComponentScopeTests.swift in Sources */,
2C15B9511C25F01200EA3486 /* ThreadSafetyTests.swift in Sources */,
0919F4E41C16419300DC3B10 /* DefinitionTests.swift in Sources */,
0919F4E31C16419300DC3B10 /* DipTests.swift in Sources */,
0919F4E51C16419300DC3B10 /* RuntimeArgumentsTests.swift in Sources */,
Expand All @@ -498,6 +504,7 @@
buildActionMask = 2147483647;
files = (
0919F4EA1C16419400DC3B10 /* ComponentScopeTests.swift in Sources */,
2C15B9521C25F01300EA3486 /* ThreadSafetyTests.swift in Sources */,
0919F4E81C16419400DC3B10 /* DefinitionTests.swift in Sources */,
0919F4E71C16419400DC3B10 /* DipTests.swift in Sources */,
0919F4E91C16419400DC3B10 /* RuntimeArgumentsTests.swift in Sources */,
Expand All @@ -519,6 +526,7 @@
buildActionMask = 2147483647;
files = (
0919F4EE1C16419500DC3B10 /* ComponentScopeTests.swift in Sources */,
2C15B9531C25F01500EA3486 /* ThreadSafetyTests.swift in Sources */,
0919F4EC1C16419500DC3B10 /* DefinitionTests.swift in Sources */,
0919F4EB1C16419500DC3B10 /* DipTests.swift in Sources */,
0919F4ED1C16419500DC3B10 /* RuntimeArgumentsTests.swift in Sources */,
Expand Down
55 changes: 41 additions & 14 deletions Dip/Dip/Dip.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ public final class DependencyContainer {
}

var definitions = [DefinitionKey : Definition]()
let lock = NSRecursiveLock()

/**
Designated initializer for a DependencyContainer

- parameter configBlock: A configuration block in which you typically put all you `register` calls.

- note: The `configBlock` is simply called at the end of the `init` to let you configure everything.
- note: The `configBlock` is simply called at the end of the `init` to let you configure everything.
It is only present for convenience to have a cleaner syntax when declaring and initializing
your `DependencyContainer` instances.

Expand All @@ -57,13 +57,35 @@ public final class DependencyContainer {
configBlock(self)
}

// MARK: - Thread safety
private func threadSafe(closure: () -> Void) {

Copy link
Owner

Choose a reason for hiding this comment

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

For code style coherence, please remove leading and trailing empty lines inside both those threadSafe implementations.

lock.lock()
defer {
lock.unlock()
}
closure()

}

private func threadSafe<T>(closure: () throws -> T) throws -> T {

lock.lock()
defer {
lock.unlock()
}
return try closure()

}
// MARK: - Reset all dependencies

/**
Clear all the previously registered dependencies on this container.
*/
public func reset() {
definitions.removeAll()
threadSafe {
self.definitions.removeAll()
}
}

/**
Expand All @@ -74,9 +96,10 @@ public final class DependencyContainer {
*/
public func remove<T, F>(definition: DefinitionOf<T, F>, forTag tag: Tag? = nil) {
let key = DefinitionKey(protocolType: T.self, factoryType: F.self, associatedTag: tag)
definitions[key] = nil
threadSafe {
self.definitions[key] = nil
}
}

// MARK: Register dependencies

/**
Expand Down Expand Up @@ -125,7 +148,9 @@ public final class DependencyContainer {
public func registerFactory<T, F>(tag tag: Tag? = nil, scope: ComponentScope, factory: F) -> DefinitionOf<T, F> {
let key = DefinitionKey(protocolType: T.self, factoryType: F.self, associatedTag: tag)
let definition = DefinitionOf<T, F>(factory: factory, scope: scope)
definitions[key] = definition
threadSafe {
self.definitions[key] = definition
}
return definition
}

Expand All @@ -138,7 +163,9 @@ public final class DependencyContainer {
*/
public func register<T, F>(definition: DefinitionOf<T, F>, forTag tag: Tag? = nil) {
let key = DefinitionKey(protocolType: T.self, factoryType: F.self, associatedTag: tag)
definitions[key] = definition
threadSafe {
self.definitions[key] = definition
}
}

// MARK: Resolve dependencies
Expand Down Expand Up @@ -190,12 +217,14 @@ public final class DependencyContainer {
let key = DefinitionKey(protocolType: T.self, factoryType: F.self, associatedTag: tag)
let nilTagKey = tag.map { _ in DefinitionKey(protocolType: T.self, factoryType: F.self, associatedTag: nil) }

guard let definition = (self.definitions[key] ?? self.definitions[nilTagKey]) as? DefinitionOf<T, F> else {
throw DipError.DefinitionNotFound(key)
return try threadSafe {
guard let definition = (self.definitions[key] ?? self.definitions[nilTagKey]) as? DefinitionOf<T, F> else {
throw DipError.DefinitionNotFound(key)
}

let usingKey: DefinitionKey? = definition.scope == .ObjectGraph ? key : nil
return self._resolve(tag, key: usingKey, definition: definition, builder: builder)
}

let usingKey: DefinitionKey? = definition.scope == .ObjectGraph ? key : nil
return _resolve(tag, key: usingKey, definition: definition, builder: builder)
}

/// Actually resolve dependency
Expand All @@ -222,9 +251,7 @@ public final class DependencyContainer {

return resolvedInstance
}

}

}

// MARK: - Private
Expand Down
28 changes: 14 additions & 14 deletions Dip/DipTests/ComponentScopeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@
import XCTest
@testable import Dip

class Server {
weak var client: Client?

init() {}
}

class Client {
var server: Server

init(server: Server) {
self.server = server
}
}

class ComponentScopeTests: XCTestCase {

let container = DependencyContainer()
Expand Down Expand Up @@ -74,20 +88,6 @@ class ComponentScopeTests: XCTestCase {
XCTAssertTrue((service1 as! ServiceImp1) === (service2 as! ServiceImp1))
}

class Server {
weak var client: Client?

init() {}
}

class Client {
var server: Server

init(server: Server) {
self.server = server
}
}

func testThatItReusesInstanceInObjectGraphScopeDuringResolve() {
//given
container.register(.ObjectGraph) { Client(server: try! self.container.resolve()) as Client }
Expand Down
17 changes: 15 additions & 2 deletions Dip/DipTests/DipTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,22 @@ extension Service {
}
}

class ServiceImp1: Service {}
internal func ==(lhs: HashableService, rhs: HashableService) -> Bool {
return lhs === rhs
}

class HashableService : Service, Hashable {
internal var hashValue: Int { get {
return unsafeAddressOf(self).hashValue
}
}
}

class ServiceImp1: HashableService {
}

class ServiceImp2: Service {}
class ServiceImp2: HashableService {
}

class DipTests: XCTestCase {

Expand Down
121 changes: 121 additions & 0 deletions Dip/DipTests/ThreadSafetyTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
//
// Dip
//
// Copyright (c) 2015 Olivier Halligon <[email protected]>
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//

import XCTest
@testable import Dip

class ThreadSafetyTests: XCTestCase {

let container = DependencyContainer()

override func setUp() {
super.setUp()
container.reset()
}

func testSingletonThreadSafety() {

let queue = NSOperationQueue()
let lock = NSRecursiveLock()
var resultSet = Set<ServiceImp1>()

container.register(.Singleton) { ServiceImp1() as Service }

for _ in 1...100 {
queue.addOperationWithBlock {
let serviceInstance = try! self.container.resolve() as Service

lock.lock()
resultSet.insert(serviceInstance as! ServiceImp1)
lock.unlock()
}
}

queue.waitUntilAllOperationsAreFinished()

XCTAssertEqual(resultSet.count, 1)
}

func testFactoryThreadSafety() {

let queue = NSOperationQueue()
let lock = NSRecursiveLock()
var resultSet = Set<ServiceImp1>()

container.register() { ServiceImp1() as Service }

for _ in 1...100 {
queue.addOperationWithBlock {
let serviceInstance = try! self.container.resolve() as Service

lock.lock()
resultSet.insert(serviceInstance as! ServiceImp1)
lock.unlock()
}
}

queue.waitUntilAllOperationsAreFinished()

XCTAssertEqual(resultSet.count, 100)
}

func testCircularReferenceThreadSafety() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also test it in a different way - buy switching thread inside resolveDependencies block of one of the definitions. Even if it's not likely to be done directly there could be cases when client code uses some method in this closure that will switch thread and at some point call another resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added some thread switching within resolveDependencies block in each threading test

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant something different, like this:

container.register(.ObjectGraph) { Server() as Server }.resolveDependencies { container, server in
  var client: Client
  dispatch_sync(dispatch_get_global_queue()) {
    client = try! container.resolve() as Client
  }
  server.client = client
}

The idea is to check that client.service is resolved correctly even if client and service are resolved on different threads.
And I meant to do it in a separate test, other tests don't need that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. I did try resolving on separate thread but without the sync wait in the dependency closure, that doesn't work as each resolve triggers another as resolved items array cleared before other thread gets to it :-) Latest should be good !


let queue = NSOperationQueue()
let lock = NSLock()

container.register(.ObjectGraph) {
Client(server: try! self.container.resolve()) as Client
}

container.register(.ObjectGraph) { Server() as Server }.resolveDependencies { container, server in
var client: Client?
dispatch_sync(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)) {
client = try! container.resolve() as Client
}
server.client = client!
}

var results = Array<Client>()

for _ in 1...100 {
queue.addOperationWithBlock {
//when
let client = try! self.container.resolve() as Client

lock.lock()
results.append(client)
lock.unlock()
}
}

queue.waitUntilAllOperationsAreFinished()

for client in results {
let server = client.server
XCTAssertTrue(server.client === client)
}
}

}