NodeJS Asynchronous

Question

I am looking for ideas/help to improve my code. It's already working, but I am not confident with it and not really proud of it. This is short version of my function -

module.exports.serverlist = async () => {
let promises = [];
const serverlist = [];
serverlist.push({ mon_sid: 'AAA', mon_hostname: 'aaaa.com', mon_port: 80 })
serverlist.push({ mon_sid: 'BBB', mon_hostname: 'bbbb.com', mon_port: 80 })

serverlist.forEach(async (Server) => {
    if (Server.mon_sid.includes('_DB')) {
        // Function home.checkOracleDatabase return promise, same as above functions
        promises.push(home.checkOracleDatabase(Server.mon_hostname, Server.mon_port));
    } else if (Server.mon_sid.includes('_HDB')) {
        promises.push(home.checkHANADatabase(Server.mon_hostname, Server.mon_port));
    } else {
        promises.push(home.checkPort(Server.mon_port, Server.mon_hostname, 1000));
    }
})

for (let i = 0; i < serverlist.length; i++) {
    serverlist[i].status = await promises[i];
}

console.table(serverlist);

What does the code do?: - It asynchronously performing needed availability check of the service.

What is the expectation? - That the code will run asynchronously, at the end of function it will wait for all promises to be resolved, for failed results it will perform the check over again, and to have more control over the promises. At this moment the promises are not really connected somehow with the array of systems, it just base on the order but it can be problematic in future.

If someone can assist/give some advises, I would be more than happy. Also I am not sure how many parallel asynchronous operations NodeJS can perform (or the OS). Currently there are 30 systems on the list, but in future it can be 200-300, I am not sure if it can be handled at once.

Update

I used promise.all - it works fine. However the problem is as I mentioned, I don't have any control over the $promises array. The results as saved in the sequence as they were triggered, and this is how they are being assigned back to serverlist[i].status array from $promises. But I would like to have more control over it, I want to have some index, or something in the $promises array so I can make sure that the results are assigned to PROPER systems (not luckily counting that by the sequence it will be assigned as it should).

Also I would like to extend this function with option to reAttempt failed checks, and for that definitely I need some index in $promises array.

Update 2

After all of your suggestion this is how the code looks for now -

function performChecks(serverlist) {
    const Checks = serverlist.map(Server => {
        if (Server.mon_sid.includes('_DB')) {
            let DB_SID = Server.mon_sid.replace('_DB', '');
            return home.checkOracleDatabase(DB_SID, Server.mon_hostname, Server.mon_port)
        } else if (Server.mon_sid.includes('_HDB')) {
            let DB_SID = Server.mon_sid.replace('_HDB', '');
            return home.checkHANADatabase(DB_SID, Server.mon_hostname, Server.mon_port);
        } else {
            return home.checkPort(Server.mon_port, Server.mon_hostname, 1000)
        }
    })

    return Promise.allSettled(Checks)
}

// Ignore the way that the function is created, it's just for debug purpose
(async function () {
    let checkResults = [];
    let reAttempt = [];
    let reAttemptResults = [];
    const serverlist = [];

serverlist.push({ mon_id: 1, mon_sid: 'AAA', mon_hostname: 'hostname_1', mon_port: 3203 })
serverlist.push({ mon_id: 2, mon_sid: 'BBB', mon_hostname: 'hostname_2', mon_port: 3201 })
serverlist.push({ mon_id: 3, mon_sid: 'CCC', mon_hostname: 'hostname_3', mon_port: 3203 })

    // Perform first check for all servers
    checkResults = await performChecks(serverlist);

    // Combine results from check into serverlist array under status key
    for(let i = 0; i < serverlist.length; i++) {
        serverlist[i]['status'] = checkResults[i].value;
    }

    // Check for failed results and save them under reAttempt variable
    reAttempt = serverlist.filter(Server => Server.status == false);

    // Perform checks again for failed results to make sure that it wasn't temporary netowrk/script issue/lag
    // Additionally performChecks function will accept one more argument in future which will activate additional trace for reAttempt
    reAttemptResults = await performChecks(reAttempt);

    // Combine results from reAttempt checks into reAttempt array
    for(let i = 0; i < reAttempt.length; i++) {
        reAttempt[i]['status'] = reAttemptResults[i].value;
    }

    // Combine reAttempt array with serverlist array so serverlist can have latest updated data
    serverlist.map(x => Object.assign(x, reAttempt.find(y => y.mon_id == x.mon_id)));

    // View the results
    console.table(serverlist);

})();

if your code is working codereview may be a better place for your question: codereview.stackexchange.com — Getter Jetter
– Getter Jetter, Commented Apr 27, 2020 at 23:10
You're going to want to use Promise.allSettled(promises) or at least Promise.all(promises) rather than that for loop at the end. — jarmod
– jarmod, Commented Apr 27, 2020 at 23:13
Wow thanks guys for so quick response! All of you, really appreciate, i already have headache with the code. I'll review all of your answers first. Also I'll edit my first post with more missing code, sorry for it, wanted you guys to avoid getting headache with all the mess code. — hapvlz
– hapvlz, Commented Apr 27, 2020 at 23:22

Yak O'Poe · Accepted Answer · 2020-04-28 16:06:04Z

2

Firstly instead of doing a for each and push promises you can map them and do a Promise all. You need no push. Your function can return directly your promise all call. The caller can await it or use then... Something like this (I didn't test it)

// serverlist declaration
function getList(serverlist) {
const operations = serverlist.map(Server => {
    if (Server.mon_sid.includes('_DB')) {
        return home.checkOracleDatabase(Server.mon_hostname, Server.mon_port);
    } else if (Server.mon_sid.includes('_HDB')) {
        return home.checkHANADatabase(Server.mon_hostname, Server.mon_port);
    } else {
        return home.checkPort(Server.mon_port, Server.mon_hostname, 1000);
    }
});

return Promise.all(operations)
}

const serverlist = [...]
const test = await getList(serverlist)

// test is an array of fulfilled/unfulfilled results of Promise.all

So I would create a function that takes a list of operations(serverList) and returns the promise all like the example above without awaiting for it. The caller would await it or using another promise all of a series of other calls to your function. Potentially you can go deeper like Inception :)

// on the caller you can go even deeper

await Promise.all([getList([...]) , getList([...]) , getList([...]) ])

Considering what you added you can return something more customized like:

    if (Server.mon_sid.includes('_DB')) {
        return home.checkOracleDatabase(Server.mon_hostname, Server.mon_port).then(result => ({result, name: 'Oracle', index:..., date:..., hostname: Server.mon_hostname, port: Server.mon_port}))

In the case above your promise would return a json with the output of your operation as result, plus few additional fields you might want to reuse to organize your data.

edited Apr 28, 2020 at 16:06

answered Apr 27, 2020 at 23:14

Yak O'Poe

8225 silver badges15 bronze badges

Sign up to request clarification or add additional context in comments.

19 Comments

hapvlz Over a year ago

Checking it right away, I already tried promise.all but the problem is as I updated in the main post, I had no control over the $promises results. it worked fine, but I never can be sure that the results are assigned to proper system in serverlist array actually.

Yak O'Poe Over a year ago

What do you mean with I don't have control of $promise? A promise.all returns an array of results in the same order as you give. You can find fulfilled promises or rejected ones when you await them (or use then). The rejected ones can be handled in other ways, you can either retry. If your lists of servers are async you need another layer on top of the function I wrote. But to be more precise I need an example of what you mean with no control. If you need more details you can simply return more data with your promise, some metadata you might reuse it to identify them (including problems).

hapvlz Over a year ago

I'll try to explain what exactly I mean with lack of control over the $promise array. At this moment trying to understand the idea that you gave me above. But I have problems with it, I am not really sure if I understand how it suppose to work. This is what I tried so far, but it doesn't give any results so far. i.imgur.com/nK5TDrG.png

hapvlz Over a year ago

// Edit seems I did a mistake, missed () on the end of (async function () {.. // Will update in second how it worked.

hapvlz Over a year ago

Jac, seems like it worked. i.imgur.com/jSrITuG.png But how should I now call the whole array? I can run it for particular serverlist[index] but if I want to run it for whole array? What I would want to achieve is, execute checks for whole array serverlist[i], and save the results in serverlist[i].status And also I want to make sure that all of the statuses are resolved. Thank you so much in advance.

|

Valentin Ivanchikov · Accepted Answer · 2020-04-27 23:09:00Z

0

For more consistency and for resolving this question i/we(other people which can help you) need all code this all variables definition (because for now i can't find where is the promises variable is defined). Thanks

answered Apr 27, 2020 at 23:09

Valentin Ivanchikov

933 bronze badges

Collectives™ on Stack Overflow

NodeJS Asynchronous

Update

Update 2

2 Answers 2

19 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

Update

Update 2

2 Answers 2

19 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related