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 3 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
88 changes: 60 additions & 28 deletions Dip/Dip/Dip.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,47 @@ public final class DependencyContainer {
}

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

/**
Designated initializer for a DependencyContainer

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

- parameter isThreadSafe: If true instance is thread safe, if false not thread safe but
slight performance gain. Default is to be thread safe.
- 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.

- returns: A new DependencyContainer.
*/
public init(@noescape configBlock: (DependencyContainer->()) = { _ in }) {
public init(isThreadSafe: Bool = true, @noescape configBlock: (DependencyContainer->()) = { _ in }) {
if isThreadSafe {
lock = NSRecursiveLock()
}
configBlock(self)
}

// MARK: - Thread safety
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()

}

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

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

}
// MARK: - Reset all dependencies

/**
Expand All @@ -74,9 +99,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 +151,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 +166,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,41 +220,43 @@ 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)
guard let definition = (threadSafe {
return (self.definitions[key] ?? self.definitions[nilTagKey])
}) as? DefinitionOf<T, F> else {
throw DipError.DefinitionNotFound(key)
}

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

/// Actually resolve dependency
private func _resolve<T, F>(tag: Tag? = nil, key: DefinitionKey?, definition: DefinitionOf<T, F>, builder: F->T) -> T {

return resolvedInstances.resolve {

if let previouslyResolved: T = resolvedInstances.previouslyResolved(key, definition: definition) {
return previouslyResolved
}
else {
let resolvedInstance = builder(definition.factory)
return threadSafe {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it will be better to lock only whole resolve<T, F>(tag tag: Tag? = nil, builder: F->T) throws -> T method that calls this one? The same will apply to other methods that will call _resolve in future (if there will be any). As for me less locking operations is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thinking about it a bit more, makes sense - less locking overhead

return self.resolvedInstances.resolve {

//when builder calls factory it will in turn resolve sub-dependencies (if there are any)
//when it returns instance that we try to resolve here can be already resolved
//so we return it, throwing away instance created by previous call to builder
if let previouslyResolved: T = resolvedInstances.previouslyResolved(key, definition: definition) {
if let previouslyResolved: T = self.resolvedInstances.previouslyResolved(key, definition: definition) {
return previouslyResolved
}

resolvedInstances.storeResolvedInstance(resolvedInstance, forKey: key)
definition.resolvedInstance = resolvedInstance
definition.resolveDependenciesBlock?(self, resolvedInstance)

return resolvedInstance
else {
let resolvedInstance = builder(definition.factory)

//when builder calls factory it will in turn resolve sub-dependencies (if there are any)
//when it returns instance that we try to resolve here can be already resolved
//so we return it, throwing away instance created by previous call to builder
if let previouslyResolved: T = self.resolvedInstances.previouslyResolved(key, definition: definition) {
return previouslyResolved
}

self.resolvedInstances.storeResolvedInstance(resolvedInstance, forKey: key)
definition.resolvedInstance = resolvedInstance
definition.resolveDependenciesBlock?(self, resolvedInstance)

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
Loading