Skip to content

Commit

Permalink
util: runtime deprecate promisify-ing a function returning a Promise
Browse files Browse the repository at this point in the history
PR-URL: #49609
Fixes: #49607
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored Sep 18, 2023
1 parent 553169f commit ccca547
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 3 deletions.
5 changes: 4 additions & 1 deletion doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -3387,12 +3387,15 @@ Consider using alternatives such as the [`mock`][] helper function.

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49609
description: Runtime deprecation.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49647
description: Documentation-only deprecation.
-->

Type: Documentation-only
Type: Runtime

Calling [`util.promisify`][] on a function that returns a <Promise> will ignore
the result of said promise, which can lead to unhandled promise rejections.
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const {
sleep: _sleep,
toUSVString: _toUSVString,
} = internalBinding('util');
const { isNativeError } = internalBinding('types');
const { isNativeError, isPromise } = internalBinding('types');
const { getOptionValue } = require('internal/options');

const noCrypto = !process.versions.openssl;
Expand Down Expand Up @@ -409,7 +409,10 @@ function promisify(original) {
resolve(values[0]);
}
});
ReflectApply(original, this, args);
if (isPromise(ReflectApply(original, this, args))) {
process.emitWarning('Calling promisify on a function that returns a Promise is likely a mistake.',
'DeprecationWarning', 'DEP0174');
}
});
}

Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-util-promisify.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,29 @@ const vm = require('vm');
const { promisify } = require('util');
const { customPromisifyArgs } = require('internal/util');

{
const warningHandler = common.mustNotCall();
process.on('warning', warningHandler);
function foo() {}
foo.constructor = (async () => {}).constructor;
promisify(foo);
process.off('warning', warningHandler);
}

common.expectWarning(
'DeprecationWarning',
'Calling promisify on a function that returns a Promise is likely a mistake.',
'DEP0174');
promisify(async (callback) => { callback(); })().then(common.mustCall(() => {
// We must add the second `expectWarning` call in the `.then` handler, when
// the first warning has already been triggered.
common.expectWarning(
'DeprecationWarning',
'Calling promisify on a function that returns a Promise is likely a mistake.',
'DEP0174');
promisify(async () => {})().then(common.mustNotCall());
}));

const stat = promisify(fs.stat);

{
Expand Down

0 comments on commit ccca547

Please sign in to comment.