Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix fs.realpath to return on error #2045

Closed
wants to merge 1 commit into from
Closed

Fix fs.realpath to return on error #2045

wants to merge 1 commit into from

Conversation

bpasero
Copy link

@bpasero bpasero commented Nov 8, 2011

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

@bnoordhuis
Copy link
Member

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
Copy link
Author

bpasero commented Nov 8, 2011

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

@bpasero
Copy link
Author

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 added a commit that referenced this pull request Nov 8, 2011
@bnoordhuis
Copy link
Member

Thanks, Benjamin. Merged in b1bb17f.

@bnoordhuis bnoordhuis closed this Nov 8, 2011
@bpasero
Copy link
Author

bpasero commented Nov 8, 2011

That was quick, thanks!

@bpasero bpasero deleted the patch-1 branch June 20, 2020 15:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants