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

Lazy InstanceWrapper can't provide thread safety #493

Open
danya61 opened this issue Aug 18, 2021 · 4 comments
Open

Lazy InstanceWrapper can't provide thread safety #493

danya61 opened this issue Aug 18, 2021 · 4 comments

Comments

@danya61
Copy link

danya61 commented Aug 18, 2021

Yesterday I faced some crash in 302:Container.swift:

        guard let currentObjectGraph = currentObjectGraph else {
            fatalError("If accessing container from multiple threads, make sure to use a synchronized resolver.")
        }

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 closure factory:

if let entry = getEntry(for: key) {
       let factory = { [weak self] in self?.resolve(entry: entry, invoker: invoker) as Any? }
       return wrapper.init(inContainer: self, withInstanceFactory: factory) as? Wrapper
} else { /**/ }

The problem is that
self?.resolve(entry: entry, invoker: invoker) as Any?
Never executed in a thread-safety way (self.resolve is not SynchronizedResolver 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:

        if let entry = getEntry(for: key) {
            let factory = { [weak self] in
                self?.lock.sync { // provide thread-safety
                    self?.resolve(entry: entry, invoker: invoker) as Any?
                }
            }
            
            return wrapper.init(inContainer: self, withInstanceFactory: factory) as? Wrapper
        } else {
            return wrapper.init(inContainer: self, withInstanceFactory: nil) as? Wrapper
        }

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.

@welshm
Copy link
Contributor

welshm commented Jul 29, 2022

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 SynchronizedResolver from a wrapper class to a property of the Container ? Then one could instantiate or set that property, so that when the Wrapper resolves it could still enforce using the locks.

@welshm
Copy link
Contributor

welshm commented Aug 4, 2022

I think this is fixed in the latest release if you want to try @danya61 @chosa91

@danya61
Copy link
Author

danya61 commented Aug 28, 2023

@welshm thanks for the solution, seems like it works correct

@dshikulin-mwb
Copy link

Then is this issue resolved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants