Add thread safety #31
Conversation
Add locking around resolution and dictionary modification to ensure thread safety
|
We removed thread safety in the latest release because it lead to deadlocks when doing circular dependencies resolution (see CHANELOG) I want to restore this thread safety as it is important but we have to be careful it doesn't generate issues with circular dependencies as demonstrated in the playground and add enough unit tests to ensure this in long term. |
|
OK - thanks for the feedback |
|
In your case I'm not sure |
|
@mwoollard what's your use case where you need container to be thread safe? |
|
@AliSoftware objc_sync_enter is recursive according to the comments in its definition header, stating it uses a recursive pthread_mutex internally. @ilyapuchka the current unit tests all passed. I can look adding some tests for thread safety if you'd like. Can run some performance comparisons with and without locking too. No specific use case right now as am just starting looking at IoC /w swift but multi-threaded access is likely as I move forward and just something I noticed. |
NSRecursiveLock is quicker than objc_sync_enter/exit so switch to it Add init parameter to disable thread safety, default is enabled
|
So tests suggest NSRecursiveLock is around 4 times more efficient than objc_sync_enter/exit (testing on 2 GHz Intel Core i7 MacBook Air). Using this as the lock approx doubles resolve time overhead (same system). Have committed some tests for threading and timing and added param to DependencyContainer init to allow thread safety to be turned off if desired. |
| } | ||
| else { | ||
| let resolvedInstance = builder(definition.factory) | ||
| return threadSafe { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, thinking about it a bit more, makes sense - less locking overhead
|
I think this thread safety parameter is an overkill. Dependency resolution is not intended to be used to resolve so many instances that it can drastically decrease performance compared to not using locks and that users would like to turn it off. For me it just makes api and implementation (optional lock) weird. |
Removed option to disable thread safety Moved locking for resolve to single lock in public resolve func Added locking in reset func
|
Yes, thread safety option not needed, was added to compare performance |
|
@AliSoftware your move, should we merge it? |
| @@ -57,13 +57,35 @@ public final class DependencyContainer { | |||
| configBlock(self) | |||
| } | |||
|
|
|||
| // MARK: - Thread safety | |||
| private func threadSafe(closure: () -> Void) { | |||
|
|
|||
There was a problem hiding this comment.
For code style coherence, please remove leading and trailing empty lines inside both those threadSafe implementations.
|
Apart from the nitpicking at code styling, LGTM 👍 Don't forget to add an entry in |
|
Before merging this there is two remaining things:
|
Add locking around resolution and dictionary modification to ensure thread safety
My understanding is Dip isn't currently thread safe when resolving, with risk of two instances of singleton being returned if same type requested at same time from different threads. How about this change to address this? I'd ideally want any container I used to be thread safe :-)
Let me know what you think !
Kind regards
Mark