util: runtime deprecate promisify-ing a function returning a Promise

[aduh95]

My goal would be for this deprecation to never reach EOL, and stay as runtime deprecated forever. The warning it produces should help folks who run into the linked issue figure out what's happening.

// file.js

const promisified = promisify(async () => {});

promisified();

node --trace-warnings file.js will show at file.js:5:1 – so not where the promisify call happened, but where the promisified function was called. Hopefully that will give enough info to debug it.

Fixes: #49607

[dnalborczyk]

I believe it is not a good pattern to rely on the constructor. The problem is also that this does not determine a function returning a Promise () => Promise.resolve(). we would probably also need to check for similar constructors, e.g. GeneratorFunction or AsyncGeneratorFunction, but I think the same issue applies here as well, it is just syntactic sugar.

[aduh95]

It's true that we will never catch all the instances where promisify is misused, but it's only emitting a warning, I don't think we need to worry too much about false negative – as long as there are no false positive, it should already be helpful.

[dnalborczyk]

True, it's just a warning, but you are preventing anyone from using async functions with a node style callback - albeit likely a bad pattern. async function foo(nodeSyleCallback) {}. That said, I believe that's not node.js' responsibility to determine. As far as I know node.js is not doing those kind of checks anywhere in the codebase -- nor should it.

[aduh95]

I'm not preventing anything, the function will still work as before. The only difference is that if it's passed to promisify, a warning will be emitted (which is not going to break anyone).
I agree that would indeed be a bad pattern, and on top of that using promisify is itself more a workaround than a great API, so all in all, I think it's fine to make that use-case less convenient.

[dnalborczyk]

I disagree, I think this is a matter of principle.

I'm not preventing anything, the function will still work as before.

ok, let me rephrase, you are nagging everyone using such a function with promisify.

and on top of that using promisify is itself more a workaround than a great API,

I don't think that's a good justification to introduce this either. 🤷‍♂️

what are you going to do if someone files an issue claiming promisify(AsyncGeneratorFunction) does not throw?

[dnalborczyk]

this does not make it an async function either:

function foo() {}
foo.constructor = 'AsyncFunction'

but the check would treat it as such.

[aduh95]

this does not make it an async function either:

function foo() {}
foo.constructor = 'AsyncFunction'

but the check would treat it as such.

It wouldn't, let me add a test to demonstrate that.

[dnalborczyk]

It wouldn't, let me add a test to demonstrate that.

ah, sorry, didn't re-check the code, I thought it was checking for the constructor name (string) actually. #49609 (comment)

Regardless, I still think this is a bad idea.

[LiviaMedeiros]

This warning makes sense, for example, in the context of node:fs and node:fs/promises.
Callback-based functions have the same names as Promise-based, and it's common to see both import fs from 'fs'; and import fs from 'fs/promises';, hence it's hard to tell what writeFile() or fs.writeFile() is without context or looking up.
Because of that, it's easy to mistakenly promisify fsPromises.writeFile or to forget to remove util.promisify while migrating from callbacks to promises.

Another error-prone thing is promisify(promisify(fn)). If util.promisify() is called on something that was already promisified by util.promisify(), it will return the once-promisified function that was stored in [kCustomPromisifiedSymbol]. However, if the first promisification will be done by any other implementation that returns AsyncFunction or rewritten as native async function, this node:util-specific mechanism will stop working without any notice.

If this warning would help someone to catch such errors, it's worth the overhead and questionable pattern.

The check can be made more strict by adding GeneratorFunction and AsyncGeneratorFunction or !== Function, but I can't come up with scenario with generator promisified by mistake.

[benjamingr]

Code LGTM, from a use case point of view I'm wondering if it wouldn't be better to warn if a promise is returned from the function rather than when a function is passed to promisify

[lukiano]

Would it be better to have something like

const returnValue = ReflectApply(original, this, args);
if (isPromise(returnValue)) {
  process.emitWarning('Calling promisify on a function that returns a Promise is likely a mistake.');
  returnValue.catch((err) => reject(err));
}

to avoid having an unhandledRejection?

const util = require('util');
function foo() {
  return Promise.reject(new Error('boom'));
}
function foo2() {
  throw new Error('boom')
}
util.promisify(foo)().catch(() => console.log('not ok'));

Ideally, we get not ok with bothfoo and foo2.

[aduh95]

@lukiano to avoid having an unhandled rejection, folks should not use promisify but call the function directly and await its result. I understand what you are trying to do, but I think it's taking it one step too far, and it would be a potential breaking change if some folks rely on the current behavior for some reason.

[aduh95]

Thinking a bit more about this, I think this naked warning is a bit weird, I've went ahead and open #49647 to make this follow the usual deprecation process, because that's more or less what this warning is: a deprecation warning. When/if the other PR lands, I'll update this one.

[lukiano]

It certainly makes me think of functions that both provide a callback and return a value. Often the value returned is a way to cancel the action (e.g. timeouts), which is addressed by AbortSignal in the promisified version.

Others, like stream.Writable.write() provide different meanings to both outputs. In this one in particular, the callback is called when the data is flushed, and the return value signals if it accepts more data. Likely a promisifed version wouldn't use either, but wait for the drain event.

Should we deprecate using promisify on any function that returns a value, even if it isn't a Promise?

[aduh95]

All functions return a value – except if it throws. What this deprecation proposal is attempting to do is help folks who "promisify" async functions, because it's likely a mistake and can be hard to debug (and as you said, rejections would not be handled). Calling promisify on a function that returns a value that might be useful is maybe a bad pattern (subjective), but it's not as likely to be a mistake, and does not have the problem of unhandled promise rejections.

[aduh95]

@nodejs/tsc since this is semver-major PRs that contain breaking changes and should be released in the next major version.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53980/

[nodejs-github-bot]

Landed in ccca547