Why is promise-limit not working in Nodejs?

user_8507120 0
Viewed 218

I have a large number of images that I want to download. I'm using the request-promise package to download the images from their URL(s). Since there were a large number of images, the server was getting overloaded and the downloads would hang or get corrupted, so I decided to use the promise-limit library to set the concurrency limit. Here is my code :


const fs = require('fs');
const request = require('request-promise');
const promiseLimit = require('promise-limit');

var limit = promiseLimit(30);

async function download(data) {
    console.log(data);

    return Promise.all(data.map( (obj) => {
        return limit(() => downloadRequest(obj))
    })).then(() => {
        console.log("All images downloaded.")
    })

function downloadRequest(obj) {
    var img = obj.dest;
    var url = obj.url;
    var req = request(url);
    req.pipe(fs.createWriteStream(img));
}

I replicated the sample exactly as is given in the github page, but the method returns without ever having the Promise.all() fulfilled. I do not understand what I am doing wrong, I thought that Promise.all() will definitely wait till it resolves all promises.

In the backend, this call is used as

...
.then((data) => {
        return downloader.download(data);
    })
    .then(() => {
        var filename = "Sample.pdf";
// Pages.json is written early in the chain, contains URL and corresponding image paths
        return generator.generate('./Pages.json', filename); 
    });

Does this mean NodeJS is already trying to generate the file out of pages.json? How can I make this part of the code synchronous to download?

promise node.js limit concurrency javascript
1 Answers

Your function downloadRequest() does not return a promise. It must return a promise that is tied to the asynchronous operation in contains such that the promise is resolved when that asynchronous operation is complete or rejected when that asynchronous operation has an error. Only when it does that can the limit() package properly do its job.

Since you're using a stream and piping it in downloadRequest(), you will have to manually construct a promise and then monitor the various events in the stream to know when it's done or has an error so you can resolve or reject that promise.

Here's an idea how to make downloadRequest() properly return a promise:

function downloadRequest(obj) {
    return new Promise((resolve, reject) => {
        const img = obj.dest;
        const url = obj.url;
        const req = request(url);

        req.on('error', reject);

        const ws = fs.createWriteStream(img);
        ws.on('error', reject);
        req.pipe(ws).on('finish', resolve);
    });
}

And, it is now recommended to use the pipeline() function instead of .pipe() because it does more complete cleanup in error conditions and there is also a promise version of that built-in:

const { pipeline } = require('stream/promises');

function downloadRequest(obj) {
    return pipeline(request(obj.url), fs.createWriteStream(obj.dest));
}

P.S. In case you didn't know, the request() library has been deprecated and it is recommended that you not use it in new projects any more. There is a list of alternative libraries to choose from here, all of which also have built-in promise support. I've looked at the various choices, tried several and decided I'm using got() in my work.

user_8166200
Related