then block isn't get called in chained promises

Question

I'm working on retrying some async calls in JS. When simplified and rewritten to setTimeout the logic looks like this:

let error = true

let promise = null

const runPromise = (value) => new Promise((res) => {
    if (!error) {
        res()
        return
    }

    if (promise) {
        return promise.then(() => {
            return runPromise(value) 
        })
    }

    promise = new Promise((res2) => {
        setTimeout(() => {
            promise = null
            console.log(value)
            error = false
            res2()
        }, 1000)
    }).then(() => res())
})

runPromise(1).then(() => { console.log(1) })
runPromise(2).then(() => { console.log(2) })
runPromise(3).then(() => { console.log(3) })

Why then blocks for runPromise(2) and runPromise(3) never got called?

probably because you are mutating the same `promise` that's shared between all of them — Federkun, Aug 15 '19 at 12:08
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Aug 15 '19 at 12:09
@Federkun yeah, it makes sense. do you have any idea how I can attach them so the chain doesn't break? the idea is to postpone any subsequent call to this `runPromise` function until the first one finishes — Vladyslav Zavalykhatko, Aug 15 '19 at 12:09
@Federkun Actually mutating global `promise` and `error` variables seems to be the whole purpose of this function? — Bergi, Aug 15 '19 at 12:15
@Bergi I don't really understand, how my code is related to the antipattern you sent link to. could you explain? — Vladyslav Zavalykhatko, Aug 15 '19 at 12:20

Bergi · Accepted Answer · 2019-08-15T12:30:06.630

Your problem is that in the if (promise) case, the promise returned by runPromise is never res()olved. returning from the executor callback doesn't do anything. You could fix this by doing

const runPromise = (value) => new Promise((res) => {
    if (!error) {
        console.log("resolve immediately without error")
        res()
    } else if (promise) {
        promise.then(() => {
            console.log("resolve after waiting for previous promise")
            res(runPromise(value))
        })
    } else {
        promise = new Promise((res2) => {
            setTimeout(() => {
                promise = null
                error = false
                res2()
                console.log("resolve after timeout")
                res()
            }, 1000)
        })
    }
})

but really you should just avoid the Promise constructor antipattern, which caused this mistake in the first place. Don't call then, new Promise or runPromise() inside that outer new Promise executor! Instead use

let error = true
let promise = null

function runPromise(value) {
    if (!error) {
        console.log(value, "resolve immediately without error")
        return Promise.resolve();
    } else if (promise) {
        console.log(value, "defer until promise")
        // now this `return` works as expected
        return promise.then(() => {
            console.log(value, "trying again")
            return runPromise(value) 
        })
    } else {
        console.log(value, "starting timeout")
        promise = new Promise(res2 => {
            setTimeout(res2, 1000)
        }).then(() => {
            promise = null
            error = false
            console.log(value, "waited for timeout")
        });
        return promise;
    }
}

runPromise(1).then(() => { console.log(1) })
runPromise(2).then(() => { console.log(2) })
runPromise(3).then(() => { console.log(3) })

@JonasWilms Yes, I was just trying to show a cleaned up version that follows the same approach as the OP and fixed the error. Whether that is a (good) solution to the actual problem of the OP (or whether he would've needed a queue) I cannot say. — Bergi, Aug 15 '19 at 12:37

Jonas Wilms · Answer 2 · 2019-08-15T12:28:08.397

You more or less have endless recursion here. Once you've set promise = the first time, if(promise) will always enter, and it will always attach another .then callback, that when called, just executes the same function again and again. It will also never call res().

I'd generally seperate concerns here: Have one function build up the promise chain and let it take a task function that gets called with the value provided:

 let queue = Promise.resolve(); // no need for null, let it always be a promise

 function runTask(task, ...args) {
  return queue = queue.then(() => task(...args));
 }

Then write another function, that represents a task, e.g. in your case it's a function that waits a tick:

 function waitTask(n) { return new Promise(res => setTimeout(res, 1000, n)); }

runTask(waitTask, 1);
runTask(waitTask, 2);

let queue = Promise.resolve();

function runTask(task, ...args) {
  return queue = queue.then(() => task(...args));
}


function waitTask(n) { return new Promise(res => setTimeout(res, 1000, n)); }

runTask(waitTask, 1).then(console.log);
runTask(waitTask, 2).then(console.log);

OP also sets `error = false` though which will make the function take the first branch, not the `if (promise)` one — Bergi, Aug 15 '19 at 12:27

then block isn't get called in chained promises

2 Answers2