0

Hi i have a problem where i'm trying to iterate over a list of keys, creating a list of matching entities The code looks like this

let previous = undefined;
for (let i = offset; i < response.keys.length; i++) {
    logger.debug("iteration " + i + " previous: " + previous)
    logger.debug("instance key: " + response.keys[i])
    if (previous) {
        previous = previous
            .then((entities) => {
                if (entities.length === limit) {
                    i = response.keys.length;
                } else {
                    readEntity(response.keys[i])
                        .then((instance) => {
                            matchInstance(instance, conditions)
                            logger.debug("is match: " + isMatch)
                                .then((isMatch) => {
                                    if (isMatch)
                                        entities.push(instance);
                                    //resolve(entities);
                                }).catch((err) => {
                                reject(err)
                            })
                        }).catch((err) => {
                        reject(err)
                    })
                }
            }).catch((err) => {
                reject(err)
            })
    } else {
        previous = readEntity(response.keys[i])
            .then((instance) => {
                logger.debug("reading instance: " + instance.key)
                matchInstance(instance, conditions)
                    .then((isMatch) => {
                        if (isMatch) {
                            return instance
                        } else {
                            logger.debug("instance does not match")
                        }
                    }).catch((err) => {
                    reject(err)
                })
            }).catch((err) => {
                reject(err)
            })
    }
}

but it only goes through the for loop once, like, it isn't returning anything?

i have some debugging aswell

2017-10-04T15:09:58+0200 <debug> data.js:202 (seekKeys.then) iteration 0 previous: undefined
2017-10-04T15:09:58+0200 <debug> data.js:203 (seekKeys.then) instance key: employees/existing@...
2017-10-04T15:09:58+0200 <debug> data.js:202 (seekKeys.then) iteration 1 previous: [object Promise]
2017-10-04T15:09:58+0200 <debug> data.js:203 (seekKeys.then) instance key: employees/test@...
2017-10-04T15:09:58+0200 <debug> data.js:202 (seekKeys.then) iteration 2 previous: [object Promise]
2017-10-04T15:09:58+0200 <debug> data.js:203 (seekKeys.then) instance key: employees/unique@...
2017-10-04T15:09:58+0200 <debug> data.js:202 (seekKeys.then) iteration 3 previous: [object Promise]
2017-10-04T15:09:58+0200 <debug> data.js:203 (seekKeys.then) instance key: employees/update@...
2017-10-04T15:09:59+0200 <debug> data.js:231 (readEntity.then) reading instance: existing@...
2017-10-04T15:09:59+0200 <debug> data.js:237 (matchInstance.then) instance does not match

please let me know if you need more info. Thank you

  • 2
    You should try to split up your code. A general rule is never to indent the code by more than one or two indents. Your code is gigantic and very difficult to troubleshoot. – OptimusCrime Oct 04 '17 at 13:28
  • 1
    I think you're misunderstanding promises. The entire loop will finish before the first promise resolves. And since you overwrite the variable 'previous' every time, only one result of the loop will be shown. Also, concerning the indents, If you return something from inside a `.then()` call that is a promsie, you can keep chaining the `.then()`s: `readEntry.then( return logger.debug ).then( ... )` – Shilly Oct 04 '17 at 13:31
  • All those `catch((err) => { reject(err) })`s hint at the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Oct 04 '17 at 15:14

2 Answers2

2

Next time it would be easier to help if, rather than just dumping a bunch of code in a question, you actually explained what you are trying to accomplish in words (along with the code). I tried to reverse engineer the objective of the code and this is what I think you were trying to do:

  1. You have an array of values in response.keys.
  2. You want to accumulate the first limit number of matches of those keys and stop looking for more matches once you have limit matches.
  3. You want to look for those matches in order of the items in response.keys.
  4. Matches are detected with readEntity(val).then() followed by a matchInstance().then() test.
  5. The desired result is an array of matched entries no longer than limit long.
  6. When an error occurs anywhere in the process, you want to abort and report the error.

Here are three separate design patterns for serializing an iteration of an array where you are doing asynchronous operations on each array element and want to do them one after another (not in parallel). Your particular case is made slightly more complicated by the requirement to stop processing after limit results are collected.

.reduce() chaining of promises

A classic design pattern for serializing promise-based async operations is to use .reduce() where the accumulator value in the reduction is a promise and you chain to that promise to force serialization. You can implement that like this:

// get subset of response.keys that we can process with .reduce()
let keys = response.keys.slice(offset);
keys.reduce((p, val) => {
    // return promise so we continually chain
    return p.then(entities => {
        // if entities is not yet full
        if (entities.length < limit) {
            return readEntity(val).then(instance => {
                return matchInstance(instance, conditions).then(isMatch => {
                    if (isMatch) {
                        entities.push(instance);
                    }
                    // resolved value is entities so that is passed down the chain
                    return entities;
                });
            });
        } else {
            return entities;
        }
    });
}, Promise.resolve([])).then(entities => {
    // process results here
}).catch(err => {
    // process error here
});

Sample implementation: https://jsfiddle.net/jfriend00/uspa8vgd/

In this promise chain, the resolved value of the promise is the array of results. It is initialized to an empty array with the Promise.resolve([]) that is passed to .reduce() as the initial accumulator value and then the resolves value of the .reduce() callback is always that same entities array. In this way, it is passed down the chain where each link in the chain has an opportunity to add to it (or not based on the isMatch test).


Manual iteration and chaining with next() function

Here's another design pattern for iterating in sequence. An internal function which I usually call next() is created. That function either returns a promise or a value. If it returns a value, then the iteration is done. If it returns a promise, then the iteration continues, with each iteration chaining onto the previous one. The eventual resolved value is whatever value you accumulated during the iteration. In here it's the entities array.

function runMatchIteration(data, offset, limit) {
    let i = offset;
    let entities = [];

    function next() {
        if (i < data.length && entities.length < limit) {
            return readEntity(data[i++]).then(instance => {
                return matchInstance(instance, conditions).then(isMatch => {
                    if (isMatch) {
                        entities.push(instance);
                    }
                    // now execute next cycle - chaining onto original promise
                    return next();
                });
            });

        } else {
            // done with loop here
            return entities;
        }
    }

    return Promise.resolve().then(next);
}


// usage
runMatchIteration(response.keys, offset, limit).then(entities => {
    // process results here
}).catch(err => {
    // process error here
});

Sample implementation: https://jsfiddle.net/jfriend00/t5bmzkb6/


Using Bluebird's Promise.mapSeries() to run things in Series

The Bluebird promise library has a number of helpful functions for managing and sequencing promises. Among those is Promise.mapSeries() which takes an array as input and serializes calling a function on each item in the array. Because your requirement to stop after limit results are achieved, we have to use it slightly different than usual, but it still makes the code fairly simple:

let entities = [];
Promise.mapSeries(response.keys.slice(offset), item => {
    if (entities.length < limit) {
        return readEntity(item).then(instance => {
            return matchInstance(instance, conditions).then(isMatch => {
                if (isMatch) {
                    entities.push(instance);
                }
            });
        });
    }
}).then(() => {
    // process entities result here
}).catch(err => {
    // handle error here
});

Some observations on your original code:

  1. You are using the Promise constructor anti-pattern where you wrap other promises inside a manually created promise rather than just return the promise you already have.
  2. When running new async operations inside a .then() handler, you must return those promises from the .then() callback so that they are chained to the parent promise. Otherwise, you create all sorts of independent unlinked promise chains and you never know when anything is finished and can't coordinate the different promise chains.
  3. Your code copies roughly the same logic into two places which is never a good idea.
  4. You can't manipulate a for loop index inside an asynchronous operation. The for loop is synchronous and will have already finished long before any of your async callbacks execute. So, the for loop is already done whenever you try to manipulate its index.
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • @ClausSandgren - I went pretty deep into rewriting your code and showing you three different techniques. Can you please at least respond and let me know if this solves your problem? – jfriend00 Oct 05 '17 at 03:23
  • Sorry i just got into work just now. I'll try out the different techniques and let you know as soon as possible! And thank you very much :) – Claus Sandgren Oct 05 '17 at 06:33
  • @ClausSandgren - OK, I guess I sleep while you work and vice/versa. – jfriend00 Oct 05 '17 at 06:34
  • I used the first technique, and it works like a charm! Thank you so much :) – Claus Sandgren Oct 05 '17 at 06:52
0

You can write a recursive function for this (not recommended):

let index = 0;
let errors = [];
function promiseIterator (index) {
    if (response.keys[index]) {
      readEntity(response.keys[index]).then((instance) => {
         matchInstance(instance, conditions).then(() => {
           logger.debug("is match: " + isMatch).then(() => {
             if (isMatch) entities.push(instance);
             promiseIterator(index+1);
           });
         });
      }).catch((err) => {
        //error handler
        errors.push({keyIndex: index, err: err});
        promiseIterator(index+1);
      });
    }
}

promiseIterator(offset);
// do your stuff with error array

The above code might not be as accurate, please tweak it according to your requirements.

But since you're required to iterate on promises, it's recommended to use bluebird library and implement each or all feature depending on your requirement.

darth-coder
  • 670
  • 8
  • 20