-
Notifications
You must be signed in to change notification settings - Fork 522
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
Lazy
InstanceWrapper can't provide thread safety
#493
Comments
This is still an issue. Our workaround currently is to resolve everything on the same thread, which is less than ideal. Would it be possible to change the |
@welshm thanks for the solution, seems like it works correct |
Then is this issue resolved? |
Yesterday I faced some crash in
302:Container.swift
:In my case, I lazily resolve dependencies, and judging by the stack trace,
Lazy<Service>
appeared in all the crashes.After a while, I seem to have found the problem.
Problem
Imagine we register some dependency with a
Lazy<Service>
argument in init:container.register(Person.self) { r in PetOwner(pet: r.resolve(Lazy<Animal>.self)!) }
Sometime later we resolve Person instance by using
SynchronizedResolver
under the hood which is looks absolutely thread safe:let person = container.synchronize().resolve(Person.self)!
After a while, we decide to use the instance:
let name = pet.instance.name
The stacktrace shows that the next step is to fall into the
resolveAsWrapper
method's closurefactory
:The problem is that
self?.resolve(entry: entry, invoker: invoker) as Any?
Never executed in a thread-safety way (
self.resolve
is notSynchronizedResolver
method)It seems to me that because of this, we can get a crash when trying to resolve a dependency from different threads at the same time (And this is probably the reason for my series of crashes).
Please correct me if my thoughts are not correct.
Possible solution
Rewrite a part of
resolveAsWrapper
method:I don't know how correct this is within such a large library, and how much it will affect the overall performance, but it should fix the multithreaded environment issue.
The text was updated successfully, but these errors were encountered: