Swinject: GraphStorage not resetting .instances on graphResolutionCompleted?

[grigorye]

Hi folks,

I'm not sure about whole thing as Swinject is still new thing to me. I have a "top-level" container that has quite a few subcontainers, instantiated as necessary for particular modules in the application (you can treat each module as a "screen of navigation"). For the reference, here's my project: https://github.com/grigorye/Weather

Nowadays I'm trying to profile the whole config for memory leaks and I believe that I figured out quite a few of them (in my implementation of resolvers and etc.).

However as part of this activity I created a test that repeats some navigation scenario in the application ("forth and back"). And while I don't see any leaks in terms of resolved instances I do see a leak traced down to GraphStorage.instances. I took a look at sources and for GraphStorage it says "Persists storage during the resolution of the object graph". Then I found "graphResolutionCompleted" and noticed that .instances are not reset there. I added instances = [:] there and it kind of resolved the issue with the "leak" to me. But probably I get it wrong and it's expected that .instances still there even after graphResolutionCompleted?

https://github.com/Swinject/Swinject/blob/6d19e0eb7ea6709a2c659806f6cff911e60e2cd0/Sources/InstanceStorage.swift#L31

[jakubvano]

Hello @grigorye,

Indeed, not reseting GraphStorage.instances in graphResolutionCompleted is intentional: we need to keep references to those instances for graph caching (see #331 for more info).

Is this problematic test case included in the project you've linked?

[grigorye]

Ah, I see. Yes, it's there in my project. You can checkout swinject-storyboard branch and take a look at 'testRepeatedNavigation' (you can lower the repeat count to something like 5): https://github.com/grigorye/Weather/blob/fae45f7148b4941dc6ca59f6979b63a426bb78be/Modules/WeatherApp/WeatherUITests/WeatherUITests.swift#L28
If you set the breakpoint at the end of the test method and break into the app, you should see quite a few malloc blocks (12 for 5 iterations, 25 for 10 iterations, 46 for 20 iterations) marked as "!" in "View Memory Graph Hierarchy" (you can use "!" filter). All of those blocks point to setInstance in GraphStorage (see the screenshot below).

Just in case, there's (increasing) number of GraphIdentifiers in "Memory Graph Hierarchy" as well, (those are not marked as leaks though).

[jakubvano]

I have been able to reproduce your results, but I'm not quite sure what causes this behaviour. I'd say that Weak<Wrapped> does not behave as expected, but haven't been able to figure out why...

[grigorye]

Great, thanks. I'll try to give it another look again as well.

[jcyu0208]

Having this same issue with my project.

Just to clarify: This issue happens when specifying both .graph and .weak scopes.

[jakubvano]

After further investigation, I came to the conclusion that this is most likely bug in memory graph analysis related to the weak references in general:

    class Foo {}
    private weak var object: Foo?

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        object = Foo()
        return true
    }

Caused an analogous leak report as test case in your project, pointing to the object = Foo() as the source.
Leak disapears if Foo inherits from NSObject.

[SolaWing]

[GraphIdentifier: Weak<Any>] entry will not release even Weak's inner object is released.
need to clear the nil entry in weak dictionary.

[rinat-enikeev]

Why don't use NSMapTable?

[jakubvano]

@SolaWing You are correct, wrapper instance will not release after its value has been, which should be fixed. However it is not directly related to the leak, I have created a separate issue for this #373.

@rinat-enikeev We are suppporting Swinject on the Linux platform, which AFAIK does not have support for NSMapTable

[jakubvano]

Closing this, as it seems like bug in memory analyzer