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)

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)

• javascript
• node.js
• promise

• if the inner find result length is not 0 what will set the loop to false? – Doron Sinai Commented Aug 11, 2015 at 17:15
• Kinda feel like all of this could be condensed down to a single query if we knew exactly what the goal of this was. mongo has a pretty powerful filtering system both for properties and sub documents. – Kevin B Commented Aug 11, 2015 at 17:23
• possible duplicate of For-loop and async callback in node.js? – doldt Commented Aug 11, 2015 at 17:24
• @KevinB actually this code is for learning (not a real project that I care about its performance right now. even if I can do it in other ways the question still remains the same) and it includes 2 queries not one which I'm not sure if I can do it in one query in Mongo but it can be interesting to see how it may work. – Arian Commented Aug 11, 2015 at 17:52
• @doldt : No , I added a ment on the last line of the post. – Arian Commented Aug 11, 2015 at 17:52

2 Answers 2

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 mon 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 ments 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 inplete 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.

As far as learning how to solve problems such as this is concerned, you might want to look at the async library (https://github./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.