New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs.exists doesn't promisify into a good fs.existsAsync #418

Closed
srguiwiz opened this Issue Dec 28, 2014 · 5 comments

Comments

Projects
None yet
3 participants
@srguiwiz
Contributor

srguiwiz commented Dec 28, 2014

Maybe should be mentioned in documentation.

Problem with promisifyAll of Node's fs for method fs.existsAsync.

Obvious reason: fs.exists doesn't callback with an err for first argument, just the result.

Workaround for many uses: Use fs.stat instead.

Example code to illustrate problem:

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

function promise(path) {
    fs.exists(path, function(exists) {
        console.log("callback with path " + path + " exists " + exists);
    });
    fs.stat(path, function(err, stats) {
        if (err) {
            console.log("stat callback with path " + path + " err " + err);
        } else {
            console.log("stat callback with path " + path + " file " + stats.isFile() + ", directory " + stats.isDirectory());
        }
    });
    return fs.existsAsync(path)
    .then(function resolve(exists) {
        console.log("promise with path " + path + " resolves exists " + exists);
    },function reject(reason) {
        console.log("promise with path " + path + " rejects reason " + reason);
    });
}

Promise.try(promise,'/');
Promise.try(promise,'/tmp');
Promise.try(promise,'/somethingimprobable');

with bluebird 2.5.0 (and 2.4.3) produces output

callback with path / exists true
promise with path / rejects reason OperationalError: true
callback with path /tmp exists true
promise with path /tmp rejects reason OperationalError: true
callback with path /somethingimprobable exists false
promise with path /somethingimprobable resolves exists undefined

which means result true gets wrapped as an error.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 28, 2014

Owner

FWIW, there should be no reason to use exists, after you have checked that the file exists, the file may disappear before you start to read it and you need the same error handling as you would have needed if you just didn't use exists in the first place.

Owner

petkaantonov commented Dec 28, 2014

FWIW, there should be no reason to use exists, after you have checked that the file exists, the file may disappear before you start to read it and you need the same error handling as you would have needed if you just didn't use exists in the first place.

@srguiwiz

This comment has been minimized.

Show comment
Hide comment
@srguiwiz

srguiwiz Dec 28, 2014

Contributor

That is a known comment. Example good use, however: At server startup after creating directory for uploads verify it exists and fail if not, e.g. lacking privileges. Don't let a server start up only to fail on first upload. mkdir doesn't give the right answer because it succeeds if newly made and fails if already exists. But, fs.stat is a tolerable workaround. I happened to convert code from callbacks, and spent some time to debug this. There was no indication fs.existsAsync wouldn't work until I spent time on it, that's why I wrote it up, so others maybe will not have to experience the same.

Contributor

srguiwiz commented Dec 28, 2014

That is a known comment. Example good use, however: At server startup after creating directory for uploads verify it exists and fail if not, e.g. lacking privileges. Don't let a server start up only to fail on first upload. mkdir doesn't give the right answer because it succeeds if newly made and fails if already exists. But, fs.stat is a tolerable workaround. I happened to convert code from callbacks, and spent some time to debug this. There was no indication fs.existsAsync wouldn't work until I spent time on it, that's why I wrote it up, so others maybe will not have to experience the same.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 28, 2014

Owner

Yes it is a common issue, I would accept pull request that mentions it in the fs promisification examples in the API documents.

Owner

petkaantonov commented Dec 28, 2014

Yes it is a common issue, I would accept pull request that mentions it in the fs promisification examples in the API documents.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Dec 29, 2014

Collaborator

in the next io.js fs.exists is officially deprecated anyway...

On Mon, Dec 29, 2014 at 1:38 AM, Petka Antonov notifications@github.com
wrote:

Yes it is a common issue, I would accept pull request that mentions it in
the fs promisification examples in the API documents.


Reply to this email directly or view it on GitHub
#418 (comment)
.

Collaborator

benjamingr commented Dec 29, 2014

in the next io.js fs.exists is officially deprecated anyway...

On Mon, Dec 29, 2014 at 1:38 AM, Petka Antonov notifications@github.com
wrote:

Yes it is a common issue, I would accept pull request that mentions it in
the fs promisification examples in the API documents.


Reply to this email directly or view it on GitHub
#418 (comment)
.

srguiwiz added a commit to srguiwiz/bluebird that referenced this issue Dec 29, 2014

srguiwiz added a commit to srguiwiz/bluebird that referenced this issue Dec 29, 2014

@srguiwiz

This comment has been minimized.

Show comment
Hide comment
@srguiwiz

srguiwiz Dec 29, 2014

Contributor

Better workaround:

fs.existsAsync = Promise.promisify
(function exists2(path, exists2callback) {
    fs.exists(path, function callbackWrapper(exists) { exists2callback(null, exists); });
 });

Example code proving it works:

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

fs.existsAsync = Promise.promisify
(function exists2(path, exists2callback) {
    fs.exists(path, function callbackWrapper(exists) { exists2callback(null, exists); });
 });

function promise(path) {
    fs.exists(path, function(exists) {
        console.log("exists callback with path " + path + " exists " + exists);
    });
    return fs.existsAsync(path)
    .then(function resolve(exists) {
        console.log("promise with path " + path + " resolves exists " + exists);
    },function reject(reason) {
        console.log("promise with path " + path + " rejects reason " + reason);
    });
}

Promise.try(promise,'/');
Promise.try(promise,'/tmp');
Promise.try(promise,'/somethingimprobable');

producing output:

exists callback with path / exists true
promise with path / resolves exists true
exists callback with path /tmp exists true
promise with path /tmp resolves exists true
exists callback with path /somethingimprobable exists false
promise with path /somethingimprobable resolves exists false
Contributor

srguiwiz commented Dec 29, 2014

Better workaround:

fs.existsAsync = Promise.promisify
(function exists2(path, exists2callback) {
    fs.exists(path, function callbackWrapper(exists) { exists2callback(null, exists); });
 });

Example code proving it works:

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

fs.existsAsync = Promise.promisify
(function exists2(path, exists2callback) {
    fs.exists(path, function callbackWrapper(exists) { exists2callback(null, exists); });
 });

function promise(path) {
    fs.exists(path, function(exists) {
        console.log("exists callback with path " + path + " exists " + exists);
    });
    return fs.existsAsync(path)
    .then(function resolve(exists) {
        console.log("promise with path " + path + " resolves exists " + exists);
    },function reject(reason) {
        console.log("promise with path " + path + " rejects reason " + reason);
    });
}

Promise.try(promise,'/');
Promise.try(promise,'/tmp');
Promise.try(promise,'/somethingimprobable');

producing output:

exists callback with path / exists true
promise with path / resolves exists true
exists callback with path /tmp exists true
promise with path /tmp resolves exists true
exists callback with path /somethingimprobable exists false
promise with path /somethingimprobable resolves exists false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment