Fixes #13 Fix threading issue in local inject #18
Conversation
ncipollo
commented
Oct 16, 2024
ncipollo
commented
- Add thread safety around local service dictionary in container.
- Adds mechanism to provide options / flags to WhoopDI so this functionality can be safely rolled out.
ncipollo
force-pushed
the
ncipollo/13/local-inject-threading-issue
branch
from
3562cd0 to
4b48d2d
Compare
| } | ||
|
|
||
| @Test(.bug("https://github.com/WhoopInc/WhoopDI/issues/13")) | ||
| func inject_localDefinition_concurrency() async { |
There was a problem hiding this comment.
If you disable the thread safety option then run this 1000 times you will hit the race condition / crash. When you enable it and do the same you should no loner see the crash.
| // We need to maintain a reference to the local service dictionary because transient dependencies may also | ||
| // need to reference dependencies from it. | ||
| // ---- | ||
| // This is a little dangerous since we are mutating a static variable but it should be fine as long as you | ||
| // This is a little dangerous since we are mutating a variable but it should be fine as long as you |
There was a problem hiding this comment.
yeah this makes me think we should lock this too
| @@ -0,0 +1,9 @@ | |||
| struct DefaultOptionProvider: WhoopDIOptionProvider { | |||
| func isOptionEnabled(_ option: WhoopDIOption) -> Bool { | |||
| false | |||
There was a problem hiding this comment.
I think we should still add the switch here and then return false
There was a problem hiding this comment.
Why? The intent is to default everything to false.
|
|
||
| final class ThreadSafeDependencyGraph: Sendable { | ||
| private let lock = NSRecursiveLock() | ||
| nonisolated(unsafe) private let serviceDict: ServiceDictionary<DependencyDefinition> = .init() |
There was a problem hiding this comment.
let's not make this nonisolated(unsafe), but instead make the class @unchecked Sendable since that is more accurate to our usage here
There was a problem hiding this comment.
Could you elaborate on why unchecked Sendable is preferable here? My rationale is that I wanted to scope this check down to just this variable since that is the only thing which is unsafe. If we were to make that a Sendable in the future we could remove this all together.
There was a problem hiding this comment.
In the way I see it, we are providing the guarantee that the ThreadSafeDependencyGraph is sendable, since we are providing locking around the non-sendable type. This to me means that this type is @unchecked Sendable because we are providing this guarantee to tell the compiler. From my understanding, nonisolated(unsafe) is more of an escape hatch where we know something won't race, but the compiler can't reason about it (but we don't really need to do anything, where here we are doing something to make it Sendable)
| Task.detached { | ||
| let _: Dependency = self.container.inject("C_Factory") { module in | ||
| module.factory(name: "C_Factory") { DependencyA() as Dependency } | ||
| } | ||
| } | ||
|
|
||
| Task.detached { |
There was a problem hiding this comment.
I think we should store both of these and await their values so we are certain that we can be certain these run. We also could do a for loop in each one of those so that we can get it to crash every time?
There was a problem hiding this comment.
That is what I had in here originally 🙃
