JS: Recursively calling a promise function

Question

I am making a node.js application that can create thumbnails for images. To avoid freezing the application while generating thumbnails I decided to use an asynchronous library for creating thumbnails. However depending on the image multiple thumbnail sizes might be needed.

var thumbnailSizes = [100];
if (image.type == 'coolImage') thumbnailSizes.push(500);
generateThumbnails(image.filename, thumbnailSizes).then(function() {
    // Do cool things with the saved thumbnails (This is never reached)
});

function generateThumbnails(filename, thumbnailSizes) {
    return new Promise(resolve => {
        var path = filename.substring(0, filename.lastIndexOf('\\'));
        console.log('start');
        console.log('length = ' + thumbnailSizes.length);
        thumb({
            prefix: thumbnailSizes[0] + '_';
            source: filename,
            destination: path,
            width: thumbnailSizes[0]
        }).then(function () {
            if (thumbnailSizes.length > 1) {
                console.log('next');
                generateThumbnails(filename, thumbnailSizes.splice(0, 1));
            } else {
                console.log('finished');
                resolve('true');
            }
        }).catch(function (e) {
            console.log('error');
        });
        console.log('end');
    });
}

This code successfully creates the first thumbnail but not the second. This is what my console looks like after the code stops running.

> Console Output
start
length = 2
end
next
start
length = 1
end

The code calls generateThumbnails() for a second time successfully but does not call the thumb function again, skipping to the end and never resolving. How can I make this work?

Instead of `resolve(generateThumbnails(filename, thumbnailSizes.splice(0, 1)));` try to just return the promise `return generateThumbnails(filename, thumbnailSizes.splice(0, 1))` — maljukan, Oct 28 '18 at 12:07
That was actually what I tried before using resolve(). Both give me the same result. — Settings Menu Hard to Find, Oct 28 '18 at 12:15
If I return thumb I get the console input `start > length = 2 > end` and no thumbnails are generated. — Settings Menu Hard to Find, Oct 28 '18 at 12:30
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Oct 28 '18 at 14:40
@maljukan `resolve(…)` is the one that *works* inside of the `new Promise` constructor, not `return …`. Of course, the `new Promise` constructor shouldn't be used at all… — Bergi, Oct 28 '18 at 14:42

score 2 · Accepted Answer · edited Oct 28 '18 at 15:33

I don't see the need for recursion here.

async function generateThumbnails(filename, thumbnailSizes) {
  var path = filename.substring(0, filename.lastIndexOf('\\'));

  return await Promise.all(thumbnailSizes.map(size => thumb({
    prefix: `${size}_`,
    source: filename,
    destination: path,
    width: size
  })));
}

Or if you need to create the thumbnails one by one:

async function* generateThumbnails(filename, thumbnailSizes) {
  var path = filename.substring(0, filename.lastIndexOf('\\'));
  for(const size of thumbnailSizes) {
    yield await thumb({
      prefix: `${size}_`,
      source: filename,
      destination: path,
      width: size
    });
  }
}

Which is consumable with a for await loop in the calling function:

for await(const thumbnail of generateThumbnails(file, sizes) {
  // handle single size
}

Also, I wouldn't use .substring() to make path manipulation, I'm sure the Node path module has a function or seven that can help you reliably extract the interesting part from the path.

While this generates my thumbnails the way I want, It also freezes the main process while it is creating them. Additionally how should I catch errors inside of `Promise.all`? — Settings Menu Hard to Find, Oct 28 '18 at 15:51
`Promise.all()` will reject on the first promise that rejects, so you can add a `try/catch` block around it (since you're in an async function), and you'll get the first error the failed. You can do some more trickery if you want *all* of the errors. As for blocking the main thread, I'm afraid that's the fault of the `thumb` function, and that there's nothing much you can do here. Consider switching to a library utilizing the new `worker_threads` module to do CPU intensive work without blocking the main thread. — Madara's Ghost, Oct 28 '18 at 15:57

score 0 · Answer 2 · answered Oct 28 '18 at 12:17

it seems you are resolving your promise with another promise, which might cause the promise chain to be broken, you can try changing:

resolve(generateThumbnails(filename, thumbnailSizes.splice(0, 1)));

for

return generateThumbnails(filename, thumbnailSizes.splice(0, 1))

However my suggestion would be (if you are using the latest ES versions) to use async/await then you don't need a recursive call and you code will be more readable:

// your function definition
//async () => {

var thumbnailSizes = [100];
if (image.type == 'coolImage') thumbnailSizes.push(500);

for(const size of thumbnailSizes) { // do not use a foreach
 const thumbnail = await generateThumbnail(image.fineName, size);
 // Do more fun stuff with your thumbnail
}

});

function generateThumbnail(filename, size) {
        var path = filename.substring(0, filename.lastIndexOf('\\'));

        return thumb({
            prefix: size + '_';
            source: filename,
            destination: path,
            width: size
        })
}

I get the error `await is only valid in async function`. I'm not sure if I can use the thumbnail function (https://github.com/honza/node-thumbnail) in the way you suggest. — Settings Menu Hard to Find, Oct 28 '18 at 12:36
Take a look at the first part of the code I wrote, you need to define your function as async: // your function definition //async () => { — Oscar Franco, Oct 29 '18 at 08:46

Bergi · Answer 3 · 2018-10-28T15:05:14.263

0

You are only calling resolve in the else block of your condition in the callback, not in the if block. Resolving the promise that the recursive call returns won't have any effect on the promise returned by the outer call. Also you never reject the promise in case of an error.

Anyway, you should avoid the Promise constructor antipattern, so that you don't need any resolve calls at all but can simply return from the then callbacks to chain promises:

function generateThumbnails(filename, thumbnailSizes) {
    console.log('length = ' + thumbnailSizes.length);
    if (thumbnailSizes.length == 0) {
        console.log('finished');
        return 'true'; // are you sure?
    } else {
        var path = filename.substring(0, filename.lastIndexOf('\\'));
        return thumb({
//      ^^^^^^
            prefix: thumbnailSizes[0] + '_';
            source: filename,
            destination: path,
            width: thumbnailSizes[0]
        }).then(function () {
            console.log('next');
            return generateThumbnails(filename, thumbnailSizes.slice(1));
//          ^^^^^^
        })
    }
}

…
var thumbnailSizes = [100];
if (image.type == 'coolImage') thumbnailSizes.push(500);
console.log('start');
generateThumbnails(image.filename, thumbnailSizes).then(function() {
    console.log('end');
    // Do cool things with the saved thumbnails (This is never reached)
}, function(err) {
    console.log('error', err);
});

Also I fixed your recursion - the base case should be the empty array.

edited Oct 28 '18 at 15:05

answered Oct 28 '18 at 14:48

Bergi

630,263
148
957
1,375

I don't know what to tell you, I'm at a complete loss. This function does not trigger `thumb()` once, and based on the console output it really should be. Also I had `generateThumbnails()` as a Promise because the "cool things with the saved thumbnails" part needs to happen after the thumbnails are created. – Settings Menu Hard to Find Oct 28 '18 at 15:03
Oops, I had missed to add the second fundamental `return` keyword. Should work now. – Bergi Oct 28 '18 at 15:06
`thumb()` is still never called. There are no errors in the console and no resolution is ever reached. Its as if that function isn't even there. – Settings Menu Hard to Find Oct 28 '18 at 15:18
So you get the `start` log, the `length = …` log with a size of 1 or more, and then nothing? That surely is weird. – Bergi Oct 28 '18 at 15:19
That is exactly what is happening. – Settings Menu Hard to Find Oct 28 '18 at 15:21
That would suggest your `thumb` function is broken. It returns a promise, but never resolves it. Did you put a breakpoint (or log statement) at the start of `thumb`? Can you post that code as well, please? – Bergi Oct 28 '18 at 15:23
`thumb` is not my function. It comes from here: https://github.com/honza/node-thumbnail/blob/master/src/thumbnail.js – Settings Menu Hard to Find Oct 28 '18 at 15:27
Oh, that's some pretty horrible code. It appears to break if you are calling `thumb` multiple times concurrently, as it uses global state. I might suggest using the `jimp` package directly. Also, did you install the error handler on the returned promise? And why are you doing promise recursion at all? node-thumbnail appears to take multiple files, its whole code is about making a queuing solution around `jimp` calls. – Bergi Oct 28 '18 at 15:36

JS: Recursively calling a promise function

3 Answers3

Linked