node-addon-api addons broken by default on Node.js 20.12+

addaleax commented 3 months ago

Version

v20.12.0

Platform

Linux 6.5.0-26-generic #26~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Mar 12 10:22:43 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

node-api

What steps will reproduce the bug?

cd /tmp && nvm install 20.12.0 && npm i --build-from-source kerberos (or any other node-addon-api addon that uses ObjectWrap or similar APIs)

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

Consistently.

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

Addon builds like it did prior to 0ac070ccb79f779390bb9944f4b802c6fc4c42aa.

What do you see instead?

npm ERR! In file included from ../node_modules/node-addon-api/napi.h:3189,
npm ERR!                  from ../src/kerberos.h:11,
npm ERR!                  from ../src/kerberos.cc:1:
npm ERR! ../node_modules/node-addon-api/napi-inl.h: In instantiation of ‘Napi::ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo&) [with T = node_kerberos::KerberosClient]’:
npm ERR! ../src/kerberos.cc:64:22:   required from here
npm ERR! ../node_modules/node-addon-api/napi-inl.h:4414:21: error: invalid conversion from ‘void (*)(napi_env, void*, void*)’ {aka ‘void (*)(napi_env__*, void*, void*)’} to ‘node_api_nogc_finalize’ {aka ‘void (*)(const napi_env__*, void*, void*)’} [-fpermissive]
npm ERR!  4414 |   status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
npm ERR!       |            ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
npm ERR!       |                     |
npm ERR!       |                     void (*)(napi_env, void*, void*) {aka void (*)(napi_env__*, void*, void*)}
npm ERR! In file included from /home/addaleax/.cache/node-gyp/20.12.0/include/node/node_api.h:12,
npm ERR!                  from ../node_modules/node-addon-api/napi.h:13,

Additional information

This was introduced in 0ac070ccb79f779390bb9944f4b802c6fc4c42aa / https://github.com/nodejs/node/pull/50060. From the PR description, this seems like intentional breakage, so it may need a solution in node-addon-api, not here. If it was intentional breakage, calling this out more explicitly somewhere in the release notes would have been helpful imo. @gabrielschulhof

mhdawson commented 3 months ago

@addaleax do you have Experiemental enabled? If I remember correctly any such change should only have affected people using the Experiemental option?

mhdawson commented 3 months ago

From the PR

In keeping with the process of introducing new Node-APIs, this feature is guarded by NAPI_EXPERIMENTAL.

so I think that my assumption that it should only affect addons using NAPI_EXPERIMENTAL is correct.

@gabrielschulhof can you clarify as well. I think you were waiting on some PRs to make it into Node.js to address issues on the node-addon-api side of things.

richardlau commented 3 months ago

If the issue is that https://github.com/nodejs/node/pull/50060 wasn't highlighted in the release notes for 20.12.0, well that had over 400 commits and https://github.com/nodejs/node/pull/50060 wasn't labelled as a notable-change (or even semver-minor). I only highlighted the equivalent in the release notes for Node.js 18.20.0 because it had an explicit backport PR.

addaleax commented 3 months ago

From the PR

In keeping with the process of introducing new Node-APIs, this feature is guarded by NAPI_EXPERIMENTAL.

so I think that my assumption that it should only affect addons using NAPI_EXPERIMENTAL is correct.

@mhdawson Yeah no, the opt-out of this behavior is experimental (behind defined(NAPI_EXPERIMENTAL) && defined(NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT)). The PR description seemed to have been accurate for an initial version of the PR, though.

do you have Experiemental enabled?

There’s a self-contained one-liner reproduction in the issue description. (Also ran this in a Docker image to make sure my personal environment is not having any effect.)

@richardlau Yeah, I was more thinking about the fact that #50060 should have been labeled as a breaking change, not that this should have been caught during the release :slightly_smiling_face:

mhdawson commented 3 months ago

Reached out to Gabriel through twitter message as well. I think there was likely a mix-up in the guards versus it being planned to affect existing addons not using NAPI_EXPERIMENTAL but would like to confirm with him.

mhdawson commented 3 months ago

@richardlau I've not managed to get in touch with @gabrielschulhof yet, but I'm wondering if we should revert the change on 20.x and 18.x. Having done the releases to do forsee any complications with that?

richardlau commented 3 months ago

@mhdawson Would it be possible for the node-api team to discuss on Friday's meeting if reverting is something that should be done and open revert PRs against 20.x and 18.x if so?

FWIW for Node.js 18.x we have already have other fixes for regressions that warrant a release:

mhdawson commented 3 months ago

@richardlau thanks for confirming that. I'll make sure that we discuss Friday in the team meeintg and if we agree we should revert I'm happy to volunteer to create the revert PRs unless that is something the releaser can easily do as part of the release process.

gabrielschulhof commented 3 months ago

@addaleax It was not our intention to break add-ons built without NAPI_EXPERIMENTAL.

At https://github.com/mongodb-js/kerberos/blob/main/src/kerberos.h#L7-L9 NAPI_EXPERIMENTAL is defined. Thus, the add-on is exposed to experimental APIs, including the new nogc types. To get the add-on to build, it is sufficient to either

run the command as CXXFLAGS='-DNODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT' npm i --build-from-source, or to
make any of the following changes to the add-on's code:
1. keep NAPI_EXPERIMENTAL and add the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT flag, or
2. bump NAPI_VERSION to 7 and remove NAPI_EXPERIMENTAL, or
3. do the work to support nogc types.

This should allow you to move past building the add-on. Thank you for being vigilant, and please keep an eye out for add-ons that break as a result of this change, because it is the first time that we're changing the API signature of otherwise stable APIs, even if we're keeping the ABI the same.

nodejs / node