Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promisify crashing node (or making it hang) #49607

Closed
SamMousa opened this issue Sep 11, 2023 · 5 comments · Fixed by #49609
Closed

Promisify crashing node (or making it hang) #49607

SamMousa opened this issue Sep 11, 2023 · 5 comments · Fixed by #49609

Comments

@SamMousa
Copy link

Version

v20.6.1

Platform

Linux xxx 6.2.0-31-generic #31-Ubuntu SMP PREEMPT_DYNAMIC Mon Aug 14 13:42:26 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Run this code:

import {promisify } from "node:util";


const asyncFunction = async () => 5;

await promisify(asyncFunction)();
console.log('This does not show');

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

Anything but a silent crash... if it's not allowed throw an error, if you do accept async functions maybe promisify should be a noop since async functions already return promises...

I'm no node developer

What do you see instead?

Status code 0, process exits.

Additional information

This bug breaks packages like this: semantic-release/commit-analyzer#525

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

Status code 0, process exits.

That's really surprising, I'm getting exit code 13, can you double check if you're seeing an exit code 0?

@SamMousa
Copy link
Author

SamMousa commented Sep 11, 2023

I can, one sec.

Edit: hmm, you are right, this reproduction is code 13, looking into source repo to see if it might be 13 there as well.

@SamMousa
Copy link
Author

SamMousa commented Sep 11, 2023

The same code crashing in the code I linked above (semantic-release/commit-analyzer#525) exits with code 0. I'm no node developer so I'm probably a lot out of my depth here...

I for sure can't catch whatever error is happening (I tried).

process.exitCode is probably the culprit of hte crash being silent, so it is crashing node but not silent by default...

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

Assuming you are seeing an exit code of 13, then it's the expected and documented behavior:

node/doc/api/process.md

Lines 3929 to 3930 in 718981e

* `13` **Unfinished Top-Level Await**: `await` was used outside of a function
in the top-level code, but the passed `Promise` never resolved.

What's happening is that promisify returns a promise which will be resolved once the last argument of asyncFunction is called. Because that never happens, the promise never settles, and the process exits because it has nothing left to do.
You need to call that last argument, e.g.:

import { promisify } from "node:util";


const asyncFunction = async (callback) => callback(null, 5);

await promisify(asyncFunction)();
console.log('This does show');

if you do accept async functions maybe promisify should be a noop since async functions already return promises...

An async function is indistinguishable from a sync function, I don't think there's a reliable way for the runtime to detect if the passed function is async or not. Maybe a linter could catch it though

@SamMousa
Copy link
Author

An async function is indistinguishable from a sync function

.constructor.name allows you to, not sure if it's standard though.

I'll look a bit further. I'm not sure the mistakes you point out mean there is no bug, or my reproduction is simply wrong...

nodejs-github-bot pushed a commit that referenced this issue Sep 18, 2023
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>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#49609
Fixes: nodejs#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants