fs: throw on invalid callbacks for async functions

If an asynchronous function is passed no callback function, there is no way to return the result. This patch throws an error if the callback passed is not valid or none passed at all. PR-URL: #12562 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
nodejs · May 9, 2017 · 4cb5f3d · 4cb5f3d
1 parent e429f9a
commit 4cb5f3d
Show file tree

Hide file tree

Showing 11 changed files with 180 additions and 104 deletions.
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -150,9 +150,10 @@ explicitly via error event handlers set on the domain instead.
 <a id="DEP0013"></a>
 ### DEP0013: fs async function without callback
 
-Type: Runtime
+Type: End-of-Life
 
-Calling an asynchronous function without a callback is deprecated.
+Calling an asynchronous function without a callback will throw a `TypeError`
+v8.0.0 onwards. Refer: [PR 12562](https://github.com/nodejs/node/pull/12562)
 
 <a id="DEP0014"></a>
 ### DEP0014: fs.read legacy String interface

diff --git a/doc/api/fs.md b/doc/api/fs.md
diff --git a/lib/fs.js b/lib/fs.js
@@ -58,7 +58,6 @@ const kMaxLength = require('buffer').kMaxLength;
 
 const isWindows = process.platform === 'win32';
 
-const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
 const errnoException = util._errnoException;
 
 function getOptions(options, defaultOptions) {
@@ -88,48 +87,26 @@ function copyObject(source) {
   return target;
 }
 
-function rethrow() {
-  // TODO(thefourtheye) Throw error instead of warning in major version > 7
-  process.emitWarning(
-    'Calling an asynchronous function without callback is deprecated.',
-    'DeprecationWarning', 'DEP0013', rethrow
-  );
-
-  // Only enable in debug mode. A backtrace uses ~1000 bytes of heap space and
-  // is fairly slow to generate.
-  if (DEBUG) {
-    var backtrace = new Error();
-    return function(err) {
-      if (err) {
-        backtrace.stack = err.name + ': ' + err.message +
-                          backtrace.stack.substr(backtrace.name.length);
-        throw backtrace;
-      }
-    };
-  }
-
-  return function(err) {
-    if (err) {
-      throw err;  // Forgot a callback but don't know where? Use NODE_DEBUG=fs
-    }
-  };
+var internalErrors;
+function lazyErrors() {
+  if (!internalErrors)
+    internalErrors = require('internal/errors');
+  return internalErrors;
 }
 
 function maybeCallback(cb) {
-  return typeof cb === 'function' ? cb : rethrow();
+  if (typeof cb === 'function')
+    return cb;
+  else
+    throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');
 }
 
 // Ensure that callbacks run in the global context. Only use this function
 // for callbacks that are passed to the binding layer, callbacks that are
 // invoked from JS already run in the proper scope.
 function makeCallback(cb) {
-  if (cb === undefined) {
-    return rethrow();
-  }
-
-  if (typeof cb !== 'function') {
-    throw new TypeError('"callback" argument must be a function');
-  }
+  if (typeof cb !== 'function')
+    throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');
 
   return function() {
     return cb.apply(null, arguments);
@@ -140,13 +117,8 @@ function makeCallback(cb) {
 // an optimization, since the data passed back to the callback needs to be
 // transformed anyway.
 function makeStatsCallback(cb) {
-  if (cb === undefined) {
-    return rethrow();
-  }
-
-  if (typeof cb !== 'function') {
-    throw new TypeError('"callback" argument must be a function');
-  }
+  if (typeof cb !== 'function')
+    throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');
 
   return function(err) {
     if (err) return cb(err);
@@ -268,10 +240,10 @@ fs.access = function(path, mode, callback) {
   if (typeof mode === 'function') {
     callback = mode;
     mode = fs.F_OK;
-  } else if (typeof callback !== 'function') {
-    throw new TypeError('"callback" argument must be a function');
   }
 
+  callback = makeCallback(callback);
+
   if (handleError((path = getPathFromURL(path)), callback))
     return;
 
@@ -280,7 +252,7 @@ fs.access = function(path, mode, callback) {
 
   mode = mode | 0;
   var req = new FSReqWrap();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.access(pathModule._makeLong(path), mode, req);
 };
 

diff --git a/test/fixtures/test-fs-readfile-error.js b/test/fixtures/test-fs-readfile-error.js
@@ -19,4 +19,4 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-require('fs').readFile('/');  // throws EISDIR
+require('fs').readFileSync('/');  // throws EISDIR
diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js
@@ -87,11 +87,11 @@ assert.throws(() => {
 
 assert.throws(() => {
   fs.access(__filename, fs.F_OK);
-}, /^TypeError: "callback" argument must be a function$/);
+}, common.expectsError({code: 'ERR_INVALID_CALLBACK'}));
 
 assert.throws(() => {
   fs.access(__filename, fs.F_OK, {});
-}, /^TypeError: "callback" argument must be a function$/);
+}, common.expectsError({code: 'ERR_INVALID_CALLBACK'}));
 
 assert.doesNotThrow(() => {
   fs.accessSync(__filename);

diff --git a/test/parallel/test-fs-link.js b/test/parallel/test-fs-link.js
@@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));
 
 assert.throws(
   function() {
-    fs.link();
+    fs.link(undefined, undefined, common.mustNotCall());
   },
   /src must be a string or Buffer/
 );
 
 assert.throws(
   function() {
-    fs.link('abc');
+    fs.link('abc', undefined, common.mustNotCall());
   },
   /dest must be a string or Buffer/
 );
diff --git a/test/parallel/test-fs-make-callback.js b/test/parallel/test-fs-make-callback.js
@@ -2,11 +2,10 @@
 const common = require('../common');
 const assert = require('assert');
 const fs = require('fs');
-const cbTypeError = /^TypeError: "callback" argument must be a function$/;
+const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
 const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
 
 const { sep } = require('path');
-const warn = 'Calling an asynchronous function without callback is deprecated.';
 
 common.refreshTmpDir();
 
@@ -17,11 +16,6 @@ function testMakeCallback(cb) {
   };
 }
 
-common.expectWarning('DeprecationWarning', warn);
-
-// Passing undefined/nothing calls rethrow() internally, which emits a warning
-assert.doesNotThrow(testMakeCallback());
-
 function invalidCallbackThrowsTests() {
   callbackThrowValues.forEach((value) => {
     assert.throws(testMakeCallback(value), cbTypeError);

diff --git a/test/parallel/test-fs-makeStatsCallback.js b/test/parallel/test-fs-makeStatsCallback.js
@@ -2,9 +2,8 @@
 const common = require('../common');
 const assert = require('assert');
 const fs = require('fs');
-const cbTypeError = /^TypeError: "callback" argument must be a function$/;
+const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
 const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
-const warn = 'Calling an asynchronous function without callback is deprecated.';
 
 function testMakeStatsCallback(cb) {
   return function() {
@@ -13,14 +12,9 @@ function testMakeStatsCallback(cb) {
   };
 }
 
-common.expectWarning('DeprecationWarning', warn);
-
 // Verify the case where a callback function is provided
 assert.doesNotThrow(testMakeStatsCallback(common.noop));
 
-// Passing undefined/nothing calls rethrow() internally, which emits a warning
-assert.doesNotThrow(testMakeStatsCallback());
-
 function invalidCallbackThrowsTests() {
   callbackThrowValues.forEach((value) => {
     assert.throws(testMakeStatsCallback(value), cbTypeError);

diff --git a/test/parallel/test-fs-mkdtemp.js b/test/parallel/test-fs-mkdtemp.js
@@ -29,8 +29,3 @@ fs.mkdtemp(path.join(common.tmpDir, 'bar.'), common.mustCall(handler));
 // Same test as above, but making sure that passing an options object doesn't
 // affect the way the callback function is handled.
 fs.mkdtemp(path.join(common.tmpDir, 'bar.'), {}, common.mustCall(handler));
-
-// Making sure that not passing a callback doesn't crash, as a default function
-// is passed internally.
-assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-')));
-assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-'), {}));
diff --git a/test/parallel/test-fs-readfile-error.js b/test/parallel/test-fs-readfile-error.js
@@ -46,7 +46,7 @@ function test(env, cb) {
 
 test({ NODE_DEBUG: '' }, common.mustCall((data) => {
   assert(/EISDIR/.test(data));
-  assert(!/test-fs-readfile-error/.test(data));
+  assert(/test-fs-readfile-error/.test(data));
 }));
 
 test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {

diff --git a/test/parallel/test-fs-write-no-fd.js b/test/parallel/test-fs-write-no-fd.js
@@ -1,12 +1,12 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const fs = require('fs');
 const assert = require('assert');
 
 assert.throws(function() {
-  fs.write(null, Buffer.allocUnsafe(1), 0, 1);
+  fs.write(null, Buffer.allocUnsafe(1), 0, 1, common.mustNotCall());
 }, /TypeError/);
 
 assert.throws(function() {
-  fs.write(null, '1', 0, 1);
+  fs.write(null, '1', 0, 1, common.mustNotCall());
 }, /TypeError/);