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)

if the inner find result length is not 0 what will set the loop to false? — Doron Sinai, Aug 11 '15 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, Aug 11 '15 at 17:23
possible duplicate of [For-loop and async callback in node.js?](http://stackoverflow.com/questions/7268579/for-loop-and-async-callback-in-node-js) — doldt, Aug 11 '15 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, Aug 11 '15 at 17:52
@doldt : No , I added a comment on the last line of the post. — Arian, Aug 11 '15 at 17:52
Would it not be a good learning experience to do things in a better way? That argument hasn't made sense to me yet. — Kevin B, Aug 11 '15 at 18:12
@KevinB : So this might not be the best solution to solve THIS specific problem , but I want to solve it in this way to be able to solve such problems which cannot be solved in any other way. does it make sense to you now ? — Arian, Aug 11 '15 at 18:35
No, not at all. When solving an issue where you're looping and performing asynchronous actions during the loop, the first step should always be deciding if you could avoid the loop all together. — Kevin B, Aug 11 '15 at 18:37
@KevinB So you mean it's never possible to have a asynchronous call in a loop ? — Arian, Aug 11 '15 at 18:39
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/86721/discussion-between-kevin-b-and-arian-hosseinzadeh). — Kevin B, Aug 11 '15 at 18:40

jfriend00 · Accepted Answer · 2015-08-11T17:28:56.190

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.

`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.` Can you please elaborate this idea ? — Arian, Aug 11 '15 at 18:01
@ArianHosseinzadeh - The containing function finishes executing and the stack is completely unwound. Then the async operation finishes and calls the callback with a clean stack. — jfriend00, Aug 12 '15 at 02:58
as soon as I find some time I will try it and will let you know — Arian, Aug 12 '15 at 21:58
It worked , but a question : We don't return anything when calling `find()` , shouldn't we do `return find()` instead ? — Arian, Aug 13 '15 at 08:05
@ArianHosseinzadeh - no. `find()` doesn't have a return value so no use in returning its return value. It calls `resolve()` or `reject()` or starts another `find()`. Those are the three ways that it ends. — jfriend00, Aug 13 '15 at 14:23

score 1 · Answer 2 · answered Aug 11 '15 at 17:46

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.

Promise inside a loop

2 Answers2

Linked