CODEWITHSUNDEEP
javascriptpromise
alex
alex

Reputation: 7601

Promise.all is never triggered due Promise rejection

I have two promises. One that reads a sample.txt file and another that reads all the files from a /books/ folder. The second promise uses a function called readFiles, which takes the dirnames and uses them to look though each file. When all the promises are ready the code inside then should run:

const p1 = new Promise((resolve, reject) => {
  fs.readdir(__dirname + '/books/', (err, archives) => {
    // archives = [ 'archive1.txt', 'archive2.txt']
    readFiles(archives, result => {
      if (archives.length === result.length) resolve(result)
      else reject(result)
    })
  })
})

const p2 = new Promise((resolve, reject) => {
  fs.readFile('sample.txt', 'utf-8', (err, sample) => {
    resolve(sample)
  })
})

Promise.all([p1, p2]).then(values => {
  console.log('v:', values)
}).catch(reason => {
  console.log('reason:', reason)
})

function readFiles (archives, callback) {
  const result = []
  archives.forEach(archive => {
    fs.readFile(__dirname + '/books/' + archive, 'utf-8', (err, data) => {
      result.push(data)
      callback(result)
    })
  })
}

However, Promise.all always get rejected:

reason: [ 'archive 1\n' ]

What am I doing wrong?

Upvotes: 0

Views: 81

Answers (1)

jfriend00
jfriend00

Reputation: 707406

Promises are one-shot devices. Once they've been rejected or resolved, their state can never change. With that in mind, readFiles() calls its callback for every file that it reads and you reject or resolve every time that callback is called, but the way you are using it, you check:

if (archives.length === result.length)

which will never be true on the first one and then you reject. Once that promise is rejected, its state cannot change. Subsequent calls to the callback will also call reject() and then the last one will call resolve(), but the state is long since set so only the first call to reject() or resolve() actually does anything. The others are simply ignored. So, p1 will always reject, thus Promise.all() that uses p1 will always reject.

You need to change readFiles() to either only call its callback once when it is done with all the files or change it to return a single promise that resolves when all the files are read or change how you're using the callback so you don't reject the first time it is called.

In general, if you're going to use promises, then you want to promisify at the lowest level and use the advantages of promises (particular for error propagation) everywhere rather than mix callbacks and promises. To that end, I'd suggest: 

fs.readFileP = function(fname, encoding) {
    return new Promise(function(resolve, reject) {
        fs.readFile(fname, encoding, function(err, data) {
            if (err) return reject(err);
            resolve(data);
        });
    });
}

function readFiles(archives, encoding, callback) {
    return Promise.all(archives.map(function(file) {
        return fs.readFileP(file, encoding);
    }));
}

Or, going a level deeper and promisifying fs.readdir() also, you'd get this:

// helper functions
fs.readdirP = function(dir) {
    return new Promise(function(resolve, reject) {
        fs.readdir(dir, function(err, files) {
            if (err) return reject(err);
            resolve(files);
        });
    });
}

fs.readFileP = function(fname, encoding) {
    return new Promise(function(resolve, reject) {
        fs.readFile(fname, encoding, function(err, data) {
            if (err) return reject(err);
            resolve(data);
        });
    });
}

function readFiles(archives, encoding) {
    encoding = encoding || 'utf8';
    return Promise.all(archives.map(function(file) {
        return fs.readFileP(file, encoding);
    }));
}

// actual logic for your operation
const p1 = fs.readdirP(__dirname + '/books/').then(readFiles);
const p2 = fs.readFileP('sample.txt', 'utf-8');

Promise.all([p1, p2]).then(values => {
  console.log('v:', values);
}).catch(reason => {
  console.log('reason:', reason);
});

If you use the Bluebird promise library which makes it easy to promisify whole modules at once and has some extra functions for managing Promise flow control, then the above code simplifies to this:

const Promise = require('bluebird');
const fs = Promise.promisifyAll(require('fs'));

const p1 = fs.readdirAsync(__dirname + '/books/').then(files => {
    return Promise.map(archives, file => {
        return fs.readFileAsync(file, 'utf8');
    });
});
const p2 = fs.readFileAsync('sample.txt', 'utf-8');

Promise.all([p1, p2]).then(values => {
  console.log('v:', values);
}).catch(reason => {
  console.log('reason:', reason);
});

In this block of code, the Promise.promisifyAll() line of code creates promisified versions of every method on the fs module with the Async suffix on them. Here, we use fs.readFileAsync() and fs.readdirAsync() so we can use promises for everything.

Upvotes: 3

Related Questions