Retain cycle

[ZoltanKapi]

Probably, most of the users will not notice it, because the instance will be stored in a static var or something similar and will live while the app.

But this strong reference causes retain cycle.

Amplitude-Swift/Sources/Amplitude/Timeline.swift

Line 11 in 0aea976

var amplitude: Amplitude?

[ZoltanKapi]

The problem is the same in EventPipeline.swift

Amplitude-Swift/Sources/Amplitude/Utilities/EventPipeline.swift

Line 11 in 1f746e8

let amplitude: Amplitude

[liuyang1520]

Thank you @ZoltanKapi for supporting Amplitude!

I think Timeline is obvious (will create a PR to update it), and I tried to use the debugging tool to generate the graph (https://stackoverflow.com/a/41661291), but I didn't find cycles for both Timeline and EventPipeline. I don't think we need weak for let in EventPipeline(https://krakendev.io/blog/weak-and-unowned-references-in-swift).

I am curious are you using any tool to detect this, would love to learn more.

Thank you!

[ZoltanKapi]

Thank you @liuyang1520 for the quick response!
Maybe I missed something, but if you have at least one strong reference for amplitude, that ref will prevents the deallocation.

Here is a dummy example:

class A {
    let b: B

    init(b: B) {
        self.b = b
    }

    deinit {
        print("deinit A")
    }
}

class B {
    var c: C

    init(c: C) {
        self.c = c
    }

    deinit {
        print("deinit B")
    }
}

class C {
    var a: A?

    deinit {
        print("deinit C")
    }
}

func factory() -> A {
    var c = C()
    var b = B(c: c)
    var a = A(b: b)
    c.a = a
    return a
}

var a: A? = factory()
a = nil

If you put this code snippet into a playground, you can't see any print in the console, because object C has a strong reference for object A, which will prevent the deallocation.
If you modify class C to store weak reference for A, it will solve the retain cycle.
This is a very simplified version, but the point is the same.

I think exactly for this reason it is not a good idea this design where the object graph is not clear tree without any loop. Apple's original solution for this problem is the delegate pattern. (you can use swiftlint to force that all delegate object should be stored in a weak property) Also with delegate protocols you can easily slice the code into smaller, more maintainable pieces.

[liuyang1520]

Thank you @ZoltanKapi for the detailed explanations!

As mentioned, I think Timeline is obvious, will update it shortly.

For EventPipeline, as Amplitude is not an optional parameter for it, I think we should keep it, references:

• https://stackoverflow.com/a/52231306
• https://krakendev.io/blog/weak-and-unowned-references-in-swift, "var vs. let"

Thanks!

[Gray-Wind]

You still can have the init with non-optional parameter, but inside it would be weak.
If you need to re-create Amplitude, you end up with leftovers of the last one, which is not good in any situation.

The strong link to the Amplitude class must be hold in once place (user-space code).

[crleona]

This should be fixed in 1.5.0