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

Already on GitHub? Sign in to your account

Fix fs.realpath to return on error #2045

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

bpasero commented Nov 8, 2011

There is a missing return statement on error for fs.realpath.

Owner

bnoordhuis commented Nov 8, 2011

Patch LGTM. Can you sign the CLA?

Sanity check: what does test/simple/test-fs-realpath.js with the below patch applied do with and without your patch?

diff --git a/test/simple/test-fs-realpath.js b/test/simple/test-fs-realpath.js
index 27ac0b8..b2c8e9b 100644
--- a/test/simple/test-fs-realpath.js
+++ b/test/simple/test-fs-realpath.js
@@ -79,6 +79,19 @@ function bashRealpath(path, callback) {
 }

 // sub-tests:
+function test_simple_error_callback() {
+  var ncalls = 0;
+
+  fs.realpath('/this/path/does/not/exist', function(err, s) {
+    assert(err);
+    assert(!s);
+    ncalls++;
+  });
+
+  process.on('exit', function() {
+    assert.equal(ncalls, 1);
+  });
+}

 function test_simple_relative_symlink(callback) {
   console.log('test_simple_relative_symlink');
@@ -415,6 +428,7 @@ function test_lying_cache_liar(cb) {
 // ----------------------------------------------------------------------------

 var tests = [
+  test_simple_error_callback,
   test_simple_relative_symlink,
   test_simple_absolute_symlink,
   test_deep_relative_file_symlink,

@piscisaureus: Can you confirm the issue?

bpasero commented Nov 8, 2011

I signed the CLA electronically. I hope its fine, otherwise let me know. Thanks.

bpasero commented Nov 8, 2011

Your test shows that the callback is being called twice, it demonstrates the issue if my patch is not applied (ncalls will be 2 in that case).

@bnoordhuis bnoordhuis added a commit that referenced this pull request Nov 8, 2011

@bnoordhuis bnoordhuis test: fs.realpath() should not call its callback twice
Test case for #2045.
6f96e78
Owner

bnoordhuis commented Nov 8, 2011

Thanks, Benjamin. Merged in b1bb17f.

@bnoordhuis bnoordhuis closed this Nov 8, 2011

bpasero commented Nov 8, 2011

That was quick, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment