Is this the correct way to write a non-blocking while loop using try-catch, setTimeout and recursion-CodePudding

I have an endpoint on my server named "/order".

When it gets triggered, I need to send an order over to an API. Sometimes, because some data over ipfs takes too long to download, the order fails within the try-catch block, and I need to resend it.

Since a while loop that tries to resend the order until it is successful would be blocking other requests, I thought I could make one myself using recursion and a setTimeout, to try to send the same request, say, 3 times every 5 minutes and if the third time it fails then it won't try again.

I wanted to know if this was the right way to achieve the non-blocking functionality and if there are some vulnerabilities/things I'm not taking into account:

async function makeOrder(body, tries) {

    if (tries > 0) {
        let order;

        try {
            order = getOrderTemplate(body.shipping_address)

            for (const index in body.line_items) {
                order.items.push(await getItemTemplate(body.line_items[index]))
            }

            await sendOrderToAPI(order)

        } catch (err) {
            setTimeout(function(){
                makeOrder(body, tries - 1)
            }, 180000)
        }
    } else {
        console.log("order n "   body.order_number   " failed")
        //write failed order number to database to deal with it later on
    }
}

CodePudding user response：

One significant problem is that, if there's a problem, it'll return a Promise that resolves not when the final API call is finished, but when the initial (failed) API call is finished. This:

        setTimeout(function(){
            makeOrder(body, tries - 1)
        }, 180000)

is not chained properly with the async function outside.

As a result, the following logic will fail:

makeOrder(body, 3)
  .then(() => {
    // All orders are made
  })

Promisify it instead, so that the recursive call can be chained with the outer function easily.

const delay = ms => new Promise(resolve => setTimeout(resolve, ms));
async function makeOrder(body, tries) {
    if (tries > 0) {
        let order;

        try {
            order = getOrderTemplate(body.shipping_address)

            for (const index in body.line_items) {
                order.items.push(await getItemTemplate(body.line_items[index]))
            }

            await sendOrderToAPI(order)

        } catch (err) {
            await delay(180_000);
            return makeOrder(body, tries - 1);
        }
    } else {
        console.log("order n "   body.order_number   " failed")
        //write failed order number to database to deal with it later on
    }
}

Another possible improvement would be to, instead of awaiting inside a loop here:

        for (const index in body.line_items) {
            order.items.push(await getItemTemplate(body.line_items[index]))
        }

to use Promise.all, or a different mechanism that allows for fetching multiple templates at once, instead of having to wait for each one-by-one in serial.

Another potential issue is that makeOrder does not reject when it exceeds the number of tries allowed. That is, the consumer wouldn't be able to do:

makeOrder(body, 3)
  .catch((error) => {
    // implement logic here to handle the error
  })

If you wanted to permit the above, at the very end of makeOrder, throw at the same time you're logging:

} else {
    console.log("order n "   body.order_number   " failed")
    //write failed order number to database to deal with it later on
    throw new Error('Failed');
}