Race condition when chaining a promise after being fulfilled?

[DNCGraef]

Hi @khanlou - first of all, thanks for all the effort putting this library together!

I've come across a potential issue which seems like a race condition inside the library. Here is code for a test:

class PromiseTests: XCTestCase {
    
    func testChainingOntoFulfilledPromiseWithOutstandingCallbacks() {
        let promise = Promise<String>()
        var didCallAlways = false
        let thenExpectation = expectation(description: "Did call 'then'")
        let alwaysExpectation = expectation(description: "Did call 'always'")
        
        promise.catch { error in
            XCTAssertEqual(didCallAlways, false)
        }.then { value in
            XCTAssertEqual(didCallAlways, false)
            thenExpectation.fulfill()
        }
        
        promise.fulfill("Hello World")
        promise.always {
            didCallAlways = true
            alwaysExpectation.fulfill()
        }
        
        wait(for: [thenExpectation, alwaysExpectation])
    }
    
}

Which intermittently fails inside

.then { value in
            XCTAssertEqual(didCallAlways, false)
            thenExpectation.fulfill()
        }

on

XCTAssertEqual(didCallAlways, false)

So, the scenario here is chaining something to the end of a fulfilled promise which has outstanding tasks chained on the end. From what I can understand stepping through the source code, I think the order in which the the "catch" & "then" tasks vs the "always" task executes is not guaranteed.

My assumption in this case was that the "always" should be queued behind the outstanding "catch" and "then" tasks.

I'm not sure if you'd consider this a bug, or a misuse of the API or something else, so thought I would raise it for discussion

Cheers

[khanlou]

Thanks for this great bug report!

intermittently fails

The two worst words to hear as a developer of a library that deals so heavily in multithreading :)

I want the behavior of the library to be as consistent as possible, so this is definitely something I'm interested in fixing. I think the issue is because of how we handle "tap"-style then blocks — those that return void. Instead of returning a new Promise, we return self, which is specifically for (if I recall correctly) not leaking resources if a promise stays in the pending state forever, especially due to an invalidatable queue.

If you have some time to explore this further, can you try returning a new promise in this function instead of self:

    @discardableResult
    public func then(on queue: ExecutionContext = DispatchQueue.main, _ onFulfilled: @escaping (Value) -> Void, _ onRejected: @escaping (Error) -> Void = { _ in }) -> Promise<Value> {
        addCallbacks(on: queue, onFulfilled: onFulfilled, onRejected: onRejected)
        return self
    }

And see if that makes the test more reliable?