-1

I have the following code which makes 10 requests to randomuser API using got and async.series. For some reasons, this only gives me one output. How do I go about to fix this?

const got = require('got');
const async = require('async');

var tenOperations = [];
for(var i = 0; i < 10; i++) {
  tenOperations.push(doRequest);
}
function doRequest(callback) {
  got('https://randomuser.me/api/')
  .then(response => {
    console.log(response.body);
  })
  .catch(error => {
    console.log(error.response.body);
  });
};

async.series(tenOperations, function(err, results) {
  if(err) console.log(err);
  console.log(results);
});

Here is the sample output:

     {
    "results": [
        {
            "user": {
                "gender": "female",
                "name": {
                    "title": "miss",
                    "first": "غزل",
                    "last": "كامياران"
                },
                "location": {
                    "street": "6186 آزادی",
                    "city": "رشت",
                    "state": "تهران",
                    "zip": 64318
                },
                "email": "غزل.كامياران@example.com",
                "username": "goldenpanda201",
                "password": "muscles",
                "salt": "OStU2tyA",
                "md5": "92ac8a84380a24785597d0e916b0174e",
                "sha1": "93f6e830538dbc557017011583cca3b5e527f854",
                "sha256": "99a4c35237b1ebe276732fbf62efca24fd457428853de8a967dd465b80b82f0f",
                "registered": 1352433856,
                "dob": 1370066399,
                "phone": "053-14062122",
                "cell": "0929-641-1309",
                "picture": {
                    "large": "https://randomuser.me/api/portraits/women/48.jpg",
                    "medium": "https://randomuser.me/api/portraits/med/women/48.jpg",
                    "thumbnail": "https://randomuser.me/api/portraits/thumb/women/48.jpg"
                }
            }
        }
    ]
}
Bun
  • 3,037
  • 3
  • 19
  • 29
  • 1
    The callback that is sent to `doRequest()` is never called by the `doRequest()` code so the async library never knows when it's done. It is still waiting for your code to call that callback before it fires the next operation in the sequence. If `got()` is already returning a promise, there are much better ways to sequence promise operations than ignoring the promise and patching a callback into it just so you can use the async library. – jfriend00 Feb 29 '16 at 21:10
  • 1
    If you can show a real example of what you really want to do (this appears to just make the exact same API call ten times in a row which is not likely of any use), we can advise on a much better way to do it. The source of data that determines how each of the 10 api calls are actually different determines how one should best code this using promises. – jfriend00 Feb 29 '16 at 21:12
  • Basically, my goal is to make 10 requests to randomuser api and store these random data to an object. I've searched around on how to do multiple requests, it seem to me that `async.series` or `async.parallel` are both convenience on this specific scenario. – Bun Feb 29 '16 at 21:16
  • 1
    **Do not use `async.js`** when you are working with promises! Hint: You're never calling `callback`. – Bergi Feb 29 '16 at 21:18

2 Answers2

1

If you just want to make ten requests to the same API in sequence (one after another, not in a parallel), you can do this:

const got = require('got');

function runSequence(url, num) {
    let cntr = 0;
    let results = [];
    return new Promise(function(resolve, reject) {

        function checkDone(data) {
            ++cntr;
            results.push(data);
            if (cntr < num) {
                next();
            } else {
                resolve(results);
            }
        }

        function next() {
            got(url).then(response => {
                console.log(response.body);
                checkDone(response.body);
            }).catch(error => {
                console.log(error.response.body);
                checkDone(null);
            });
        }
        next();
    });
}

runSequence('https://randomuser.me/api/', 10).then(function(results) {
    // access array of results here
});

If the API calls don't have to be done one at a time and you can have them all in flight at the same time, then you can do this:

function runParallel(url, num) {
    let promises = [];
    for (let i = 0; i < num; i++) {
        promises.push(got(url));
    }
    return Promise.all(promises);
}

runParallel('https://randomuser.me/api/', 10).then(function(results) {
    // access array of results here
});

Note: The parallel option aborts on the first error, whereas both the sequence options shown here continue on error. Either could be changed to the other behavior. You did not specify which you wanted.


Here's a slightly different way of running the sequence:

function runSequence(url, num) {
    let cntr = 0;
    let results = [];

    function checkDone(data) {
        ++cntr;
        results.push(data);
        if (cntr < num) {
            return next();
        } else {
            return results;
        }            
    }

    function next() {
        return got(url).then(response => {
            return checkDone(response.body);
        }).catch(error => {
            return checkDone(null);
        });
    }

    return next();
}

Here's a generic function for repeating some async operation N times. You pass in the async function (that returns a promise), the number of times you want it repeated in sequence and whether you want it to continue on error or abort on error.

// pass a function that returns a promise
function repeatSequence(fn, num, continueOnError) {
    let cntr = 0;
    let results = [];

    checkDone(data) {
        ++cntr;
        results.push(data);
        if (cntr < num) {
            return next();
        } else {
            return results;
        }
    }

    function next() {
        return fn().then(checkDone).catch(function(err) {
            if (continueOnError) {
                return checkDone(null);
            } else {
                // reject on error
                throw err;
            }
        });
    }

    return next();
}

And, if you use the Bluebird Promise library, you can make use of Promise.mapSeries() with this:

function repeatSequence(fn, num, continueOnError) {
    var array = new Array(num);
    return Promise.mapSeries(array, function () {
        return fn().catch(function (err) {
            if (continueOnError) {
                return null;
            } else {
                throw (err);
            }
        });
    });
}

Or, if you don't want the continueOnError option, it just becomes this:

function repeatSequence(fn, num) {
    var array = new Array(num);
    return Promise.mapSeries(array, fn);
}
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • 1
    I added a parallel version too. – jfriend00 Feb 29 '16 at 21:27
  • 1
    @Bergi - I presume you're talking about the `new Promise()` in the first option. It's often easier to create a new master promise when you are using some involved logic programming to determine whether you're continuing or done. I don't see what the issue is with coding the way that is simpler. There is no programming objective to use the fewest promises possible. There is a programming objective to code as simple as possible that is robust, readable, maintainable, etc... Sometimes those two purposes conflict. I have added another option, but I'm not sure it's actually simpler to understand. – jfriend00 Feb 29 '16 at 21:48
  • 1
    Yes, that's what I'm talking about. Well, it might seem simpler on the first look, but unfortunately it's wrong - can you spot the mistake that makes it behave differently from the fixed version? (If you can't, that's exactly the problem, and the very reason why this pattern has to be extinguished [*insert evangelistic rant here*]) – Bergi Feb 29 '16 at 21:50
  • 1
    @Bergi - Good catch. I fixed an error in the first scheme. Not sure if it's the error you were referring to. The same error I fixed in the first scheme was a problem in the last scheme too (so I fixed it there also). – jfriend00 Feb 29 '16 at 21:53
  • 1
    Yes, never settling the promise in the case of an error was what I meant. That pattern is just too error-prone (while, at the same time, often enough still working when there are no runtime problems), even when written by experienced users. – Bergi Feb 29 '16 at 21:57
  • 1
    I added a generic sequencer function and cleaned up the other implementations a bit. – jfriend00 Feb 29 '16 at 22:06
  • @Bergi - Yeah, I see your point on the error-prone-ness. The whole concept of returning a promise from inside a `.then()` handler to automatically chain to the prior promise is very powerful, but also not simple for newbies to grasp. I sometimes feel conflicted here on Stack Overflow between presenting an "advanced" solution and an "easiest to follow" solution. I decided to offer both here. – jfriend00 Feb 29 '16 at 23:26
  • 1
    @Bun - Please note the code corrections I've made to my answer in case you grabbed what I first had here. – jfriend00 Feb 29 '16 at 23:27
  • 1
    Added smaller code option using Bluebird's `Promise.mapSeries()`. – jfriend00 Mar 01 '16 at 00:12
  • Thanks. I got it. :D @jfriend00 – Bun Mar 01 '16 at 01:15
1

I must agree with Bergi that we must not mix callbacks with promises. Even though they are both async mechanisms, each of these two constructs are essentially and philosophically not to be treated the same.

However, as jfriend00 pointed out, the primary problem was that the "callbacks" were not being called, in each step. This would have been the correct solution while using a non-promise library.

Solution

Please find below my modifications. The essence would be:

  1. This solution is an example of how you might implement it using promises only.
  2. Ports like async-q might help you play better with existing libraries.

    const got = require('got');
    const async_q = require('async-q');
    
    var tenOperations = [];
    for(var i = 0; i < 10; i++) {
        tenOperations.push(doRequest);
    }
    
    function doRequest() {
        return got('https://randomuser.me/api/')
        .then(response => {
            console.log('resp', response.body);
            return JSON.parse(response.body);
        })
        .catch(error => {
            console.log('error', error.response.body);
            return error.response.body;
        });
    }
    
    async_q
    .series(tenOperations)
    .then (results => {
        console.log('results', results);
    })
    .done();
    
ejhari
  • 11
  • 2