Investigate possible retain cycles passing container in factory/resolveDependenciesBlock

[ilyapuchka]

As @AliSoftware pointed out in #22 discussion there could be some retain cycles between container and definitions. Will investigate this possibility in this issue.

---

Discussion from #22 :

AliSoftware:

Not related to this PR I know, but now that I read that again I'm wondering: shouldn't we use [unowned container](or at least [weak container]) here? It seems to me that container strongly reference resolveDependenciesBlock which itself strongly references container, hence a strong reference cycle, right?

ilyapuchka:

I don't think so, cause here we don't capture container from outer context, we pass it as an argument at runtime. So the reference should be released as soon as block returns. I suppose...

Speaking of unowned I tried to use factory block without [unowned container] it (like in line 93) and it still looks like there is no retain cycle... I test it by having a strong reference to container and a weak reference. When I release a strong reference weak also is being released. That would not happen in retain cycle... but if I think of it there should be one if there is no [unowned container] or [weak container], cause container holds definitions and they hold their factories... Maybe it's just too late at night to think about that 😪 🌃

AliSoftware:

I don't think so, cause here we don't capture container from outer context, we pass it as an argument at runtime. So the reference should be released as soon as block returns.
Don't we? I mean, container captures resolveDependenciesBlock right? So unless that block don't use the container argument in the closure, this container is captured automatically by the closure and not "released as soon as the block returns" (as the resolveDependenciesBlock will still be captured and kept as a property in the container). So this captured container will only be released when the block itself is released (set back to nil).

(If the block were not stored as a strong property by the container but used right away, we would be able to mark it @NoEscape, which is not the case here)

[ilyapuchka]

I'm still quite confident that passing container as an argument in runtime is ok. Otherwise we would need to wrap each call to resolveDependenciesBlock in dummy closure so that we can capture container as unowned and pass it to the closure? Sounds not real... Can't think of any other way to capture container in this case. Closures should work the same way as obj-c block, but never done such "dance" there.

{ [unowned self] in
 definition.resolveDependenciesBlock?(self, resolvedInstance) 
}()

The case with capturing container in factory block is more likely will lead to retain cycle, so I will write some unit tests to check that or try out some sample code.

[ilyapuchka]

I tried it with a simple playground code. No retain cycles at all, even if capturing container strongly in factory block. That I can not explain.

Here is a gist - https://gist.github.com/ilyapuchka/8d0690101f23c6d09225
There A class stands for Dip definitions, B stands for container, factory - is a definition factory, closure is for resolveDependencyBlock.

Am I missing something?

[AliSoftware]

Mmhh not sure I understand it either. what sorcery is this?!

[ilyapuchka]

At one moment I thought it could be that array is a struct and it change the rules somehow (but still there should be strong references to each of it's items anyway) but I tried with NSArray - the same.

[AliSoftware]

Just asked around on Slack will see if someone has clearer thoughts than we do!

[ilyapuchka]

Maybe swift is smart enough to solve such kind of cases. Cause there are something happening under the hood that we don't know, like this - https://twitter.com/jckarter/status/655031895122579456

[ilyapuchka]

Everything is clear now - https://twitter.com/jckarter/status/671812668005584896
If instead we would have something like this:

 func addA() -> A {
        let a = A(factory: {
            print("factory \(self)")
        })
        b.array.append(a)
        return a
    }

then it will create a retain cycle.
So no problem in our case also and [unowned container] is not needed both in factory and resolveDependencyBlock of DefinitionOf. Better to remove it from docs to make it more clean and maybe reference this issue?

[AliSoftware]

Ahhh got it. Tricky one still.
Yeah better remove it from doc and reference issue indeed. Well done!

[ilyapuchka]

Yep, though I messed up with variables and used b instead of self in addA, that does not change the whole thing.