LazyInjected, WeakLazyInjected causing memory leak

[ZsoltMolnarMBH]

Simplified example:

extension Resolver {
    static var context: Resolver!
}

class A { }

class B {
    @LazyInjected(container: .context) var a: A
}

func assemble(container: Resolver) {
    container.register { A() }
    container.register { resolver -> B in
        Resolver.context = resolver
        defer { Resolver.context = nil }
        return B()
    }
    .scope(.container)
}

// Example viewcontroller

import UIKit
import Resolver

class ViewController: UIViewController {
    override func viewDidLoad() {
        super.viewDidLoad()
        let container = Resolver(child: .root)
        assemble(container: container)
        let _ = container.resolve(B.self)
    }
}

Experienced: Memory leak occours due to the following circular references: container -> container.cache -> B -> B.@LazyInjected(container: .context) var a: A -> container

Suggested fix: A possible way to resolve this is to mark the public var container: Resolver? property inside LazyInjected and WeakLazyInjected as weak.

[hmlongco]

Based strictly on that code I think you've got another one...

container -> registration (captures container) -> container

[ZsoltMolnarMBH]

Based strictly on that code I think you've got another one...

container -> registration (captures container) -> container

Hello! I think the registration factory closure doesn't capture the container, I updated the example code, to make it more clear.

[hmlongco]

Ummm.... you may need to take a more active role in ownership and management.

class ViewController: UIViewController {
    let container = Resolver(child: .root)
    override func viewDidLoad() {
        super.viewDidLoad()
        assemble(container: container)
        let _ = container.resolve(B.self)
    }
    deinit {
        container.cache.reset()
    }
}

I'll give it some thought, but at the moment I'm disinclined to make the wrapper container weak as that could preclude something like...

class C {
    @LazyInjected(container: dynamicallyRegisteredContainer()) var a: A
}

Where the wrapper would actually want to hold onto the container.

[ZsoltMolnarMBH]

We would like to avoid having to reset the container cache manually. As far as I can see, it is a bit of a semantical error that a property wrapper has a strong reference to a container. I think using this link to keep up any kind of hierarchy is wrong, since the container is kind of the parent of the component. I cannot think of a setup where this is not some kind of abuse.

What do you think about reducing this property to weak? It would solve our problem, and I think it would be semantically correct aswell.

[ZsoltMolnarMBH]

Hello @hmlongco , Could you please give us an update on this?

[hmlongco]

As I said earlier, I don't think I want to make that change for the reason mentioned, as well as the fact that allowing a container to go out of scope behind the scenes could cause a crash if that lazy property wrapper ever attempts to resolve.

Generally speaking, that link isn't an issue under normal conditions because containers tend to be statically implemented on Resolver itself and they persist for the lifetime of the app. Also, making that change might fix your problem, but could also inadvertently break someone else's code that depends on existing behavior.

You have a couple of work-arounds for this, including not using Lazy, doing better management of the circular caching condition you setup in the first place, and, for that matter, even cloning the property wrappers.

[ZsoltMolnarMBH]

"containers tend to be statically implemented on Resolver itself", the unspoken part here is that, you expect that user defined containers are expected to be static too. - I think nor code or documentation suggests that this is the intended way. Even if this was added to the documentation, it wouldn't have a solid foundation.

About the (PW->Container) retaining link. I still think this is an obvious semantical error. Fixing this with a weak link would only require incrementing major version number (yes I agree that this could be a braking change for some).

Reimplementing property wrappers would be a major inconvenience, especially since some of them are using private classes.

Any way, seems like we have come to a disagreement, which I will accept. Thank you for the investigation! Feel free to close the issue, we will come up with something on our end.