Promise inside a loop

Question

In the following code, I have an infinite loop which I don't know why it happens. My best guess is because the function inside is async the loop doesn't wait for it and so the loop never stops. What is the best way to solve this issue ?

 var generateToken = function(userId) {
    return new Promise(function(resolve, reject) {
        User.findOne({userId: userId}, function(err, user) {
            if (user !== null) {
                var loop = true;
                while (loop) {
                    var token = Common.randomGenerator(20);
                    (function(e) {
                        User.find({tokens: e}, function(err, result) {
                            if (err) {
                                loop = false;
                                reject('Error querying the database');
                            } else {
                                if (result.length === 0) {
                                    if (user.tokens === undefined) {
                                        user.tokens = [];
                                    }
                                    user.tokens.push(e);
                                    loop = false;
                                    resolve();
                                }
                            }
                        });
                    })(token);
                }
            } else {
                return reject('UserNotFound');
            }
        });
    });
};

This function receives a userId (User.findOne() is used to find the user and if there's no user with that id , reject the promise) and creates a unique random token for that user (randomGenerator) , add it to the user entity which is kept in MongoDB and the return it back to the caller.

(NOTE There were some down votes saying this question is same as this one Which is not as I already had a closure in my code and it still doesn't work. That question was more about how to bind the looping variable to the closure)

Answer 1

You are correct that you cannot loop like you are trying to do.

Javascript is single threaded. As such as long as the main thread is looping in your while(loop) statement, NOTHING else gets a chance to run. This would all be OK if your main thread itself was changing the loop variable, but that isn't exactly what is happening. Instead, you're trying to change the loop variable in an async response, but because your main thread is looping, the async response can NEVER be processed so thus your loop variable can never get changed and thus a non-productive infinite loop.

To fix, you will have to change to a different looping construct. A common design pattern is to create a local function with the code in it that you want to repeat. Then, run your async operation and if, in the async result handler, you decide you want to repeat the operation, you just call the local function again from within it. Because the result is async, the stack has unwound and this isn't technically recursion with a stack build up. It's just launching another iteration of the function.

I am a bit confused by the logic in your code (there are zero comments in there to explain it) so I'm not entirely sure I got this correct, but here's the general idea:

var generateToken = function(userId) {
    return new Promise(function(resolve, reject) {
        User.findOne({userId: userId}, function(err, user) {
            function find() {
                var token = Common.randomGenerator(20);
                User.find({tokens: e}, function(err, result) {
                    if (err) {
                        reject('Error querying the database');
                    } else {
                        if (result.length === 0) {
                            if (user.tokens === undefined) {
                                user.tokens = [];
                            }
                            user.tokens.push(e);
                            resolve();
                        } else {
                            // do another find until we don't find a token
                            find();
                        }
                    }
                });
            }

            if (user !== null) {
                find();
            } else {
                reject('UserNotFound');
            }
        });
    });
};

I should note that you have incomplete error handling on the User.findOne() operation.

FYI, the whole logic of continuously querying until you get result.length === 0 just seems bizarre. The logic seems odd and it smells like "polling a database in a tight loop" which is usually a very poor performing thing to do. I suspect there are much more efficient ways to solve this problem if we understood the overall problem at a higher level.

Answer 2

As far as learning how to solve problems such as this is concerned, you might want to look at the async library (https://github.com/caolan/async). It provides pretty intuitive ways of handling asynchronous situations like this, in a way that can be understood by most people familiar with synchronous iteration and the basics of javascript, for almost any style of asynchronous iteration you can imagine, and is widely used and very well documented.