Promise does not deinit about then HOT 15 CLOSED

Future investigations, removing in registerThen<X>

            self.progressBlocks.append({ p in
                progress(p)
            })

totally heal leaks

+
+
+
.
+
+
+
-
-
-
+
.
-
-
-
-

from then.

@mandarin6b0 Thanks a ton for your time and investigations,
I'm will take a look whenever I have some time and let you know how it goes.
Cheers,

from then.

@mandarin6b0 Indeed I can reproduce on my side. Have no idea how we didn't see this issue earlier :)
in registerThen<X> replacing

self.progressBlocks.append({ p in
  progress(p)
})

by

self.progressBlocks.append({ p in
  //progress(p)
})

solves this issue. So our problem here seems to lie in capturing the progress callback in a block

I have no solution right now but I'm on it ! :) If you have any ideas, feel free to share.

Have a good day,

from then.

@s4cha, thank you for reply. Solved cycle by changing in start() method

p(resolve:resolvePromise, reject:rejectPromise, progress:progressPromise)

to

p(resolve: { [weak self] v in self?.resolvePromise(v) },
    reject: { [weak self] v in self?.rejectPromise(v) },
    progress: { [weak self] v in self?.progressPromise(v) })

There may be more places with cycled refs, need some more time for audit.
P.S. I'm using swift 2 now in my project, so the the code will be different for swift 3

from then.

I did it on swift 3 and had the same conclusions, I will try your fix tomorrow. That sounds awesome!! Great work @mandarin6b0

from then.

Hey @mandarin6b0, I found that replacing this :

let p = Promise<X> { resolve, reject in
    switch self.state {
    case let .fulfilled(value):
        let x: X = block(value)
        resolve(x)
    case let .rejected(error):
        reject(error)
    case .pending:
        self.successBlocks.append({ t in
            resolve(block(t)) //Because P1 captures P2's resolve block
        })
    }
}
p.start()

by this :

let p = Promise<X>()
switch state {
case let .fulfilled(value):
    let x: X = block(value)
    p.resolvePromise(x)
case let .rejected(error):
    p.rejectPromise(error)
case .pending:
    self.successBlocks.append({ t in
        p.resolvePromise(block(t))
    })
}
p.start()

in @discardableResult public func then<X>(_ block: @escaping (T) -> X) -> Promise<X> { solves the retain cycle.
The problem with the previous fixes discussed before is that they break the unit tests.

By the way, what you found can also be shown with the code below :

    func foo() {
        let a = Promise<String> { r, _ in
            Timer.scheduledTimer(withTimeInterval: 3, repeats: false, block: { _ in
                r("TOTO")
            })
        }

        a.then{ s in
            print("Doing 1 thing")
        }
    }

In essence, if promise a is async then when the second registers on it, its state is still pending, which cause the latter to call

self.successBlocks.append({ t in
    resolve(block(t)) //Because P1 captures P2's resolve block
})

which seems to cause the retain issue.

We're getting closer :)

Cheers,

from then.

@mandarin6b0
Ok I think I found the missing piece of the puzzle.

The retain cycle is, as you noticed earlier, in part due to promise.initialPromiseStart = self.start

Indeed each promises captures the next ones resolve and reject blocks but also each keep a reference to the first one, hence the cycle.

The fix
promise.initialPromiseStart = { [weak self] self?.start() }
breaks the retain cycle (yay) but it introduces and issue.

Since it's a weak reference, it might actually be released before we even use it! :(

For instance when we're using registerThen we are waiting to be triggered so we need to keep a strong reference

promiseA().registerThen { s in
    print("Doing 1 thing")
}.then {
    print("Doing 2 things")
}

would not work unless we write it like so :

let a = promiseA()
a.registerThen { s in
    print("Doing 1 thing")
}.then {
    print("Doing 2 things")
}

Because nothing retains promiseA() in case 1.

So it made me think, what if we release the initial promise ourselves, but only once we don't need it anymore.
so essentially calling initialPromiseStart = nil in func resolvePromise(_ result: T)

And this solves the retain cycle, and the tests pass \o/. :)

Cheers

from then.

@mandarin6b0 here is the diff that fixes the issue on my side :
https://gist.github.com/s4cha/7f03a415a147b1eb2c1e05c59111be47

It essentially is :

• Using [weak self] in promise blocks
• Setting initialPromiseStart = nil in resolvePromise and rejectPromise

This is on swift 3 but should be the same for swift 2.

Let me know how it works for you and we'll update the version :)

Have a fantastic day and thanks again for your work 👏 !!!

from then.

@s4cha something is still wrong on my side, I applied your diff:
https://gist.github.com/mandarin6b0/41d313bf540e501b99801136c3741b38
However the output shows no retains :(
Are start and passAlongFirstPromiseStartFunctionAndStateTo methods still the same on your side?

from then.

@mandarin6b0 Indeed :/

for some reason this works :

        let a = Promise { r, _ in
            Timer.scheduledTimer(withTimeInterval: 1, repeats: false, block: { _ in
                r()
            })
        }

        a.then { print(".") }

while this doesn't :

        let a = Promise { r, _ in
            r()
        }

        a.then { print(".") }

aka async vs sync

Back to work then ^^

from then.

@mandarin6b0 of think I got it, look like moving self?.progressBlocks.append(progress) into the pending: case only fixes the issue :)

diff --git a/Source/Promise.swift b/Source/Promise.swift
index 9799cd5..dc5ca36 100644
--- a/Source/Promise.swift
+++ b/Source/Promise.swift
@@ -78,8 +78,8 @@ public class Promise<T> {
                         resolve(block(t))
                     })
                     self?.failBlocks.append(reject)
+                    self?.progressBlocks.append(progress)
                 }
-                self?.progressBlocks.append(progress)
             }
         }
         p.start()

Is it ok on your side ?

from then.

Here is a sample project with your issue :)
testLEAKthen.zip

from then.

@s4cha yes, moving progress block fix it. All ok now on my side :)

from then.

Hey @mandarin6b0 this is now live in 2.0.1 -> https://github.com/freshOS/then/releases/tag/2.0.1
Thanks a ton for your work an determination to fix it, very much appreciated!

from then.

@s4cha, 👍 Thanks you for your work! Then I'm closing this issue :)

from then.