[v6.x backport] src: add environment cleanup hooks

[gabrielschulhof]

This adds pairs of methods to the Environment class and to public APIs
which can add and remove cleanup handlers.

Unlike AtExit, this API targets addon developers rather than
embedders, giving them (and Node’s internals) the ability to register
per-Environment cleanup work.

We may want to replace AtExit with this API at some point.

Many thanks for Stephen Belanger for reviewing the original version of
this commit in the Ayo.js project.

Refs: ayojs/ayo#82
PR-URL: #19377

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@gabrielschulhof build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/752/pipeline

[gabrielschulhof]

@addaleax I had to add a TODO because attempting to clean up the handles upon exit was causing a busy loop. Looks like the handle close callback wasn't getting called in CleanupHandles().

[gabrielschulhof]

Re nodejs/abi-stable-node#330

[addaleax]

I think just not doing this is a perfectly viable option. We’re not actually interested in making the Environment clean up after itself, that’s probably impossible at least for v6.x.

[addaleax]

@gabrielschulhof The PR looks good to me, but be aware that afaik we generally don’t do semver-minors on maintenance release lines.

[gabrielschulhof]

@addaleax thanks for the review! We discussed it in @nodejs/n-api and I think we'll ask nicely for an exception so we can retain our claim that N-API version 3 is implemented across all supported versions.

[addaleax]

@nodejs/lts I’m adding the label for your agenda ^^^

[richardlau]

v6.x is in Maintenance mode https://github.com/nodejs/Release/blob/master/README.md:

Once a release moves into Maintenance mode, only critical bugs, critical security fixes, and documentation updates will be permitted.

Perhaps we should start a discussion on n-api fixes/changes going into Maintenance releases?

[MylesBorins]

I'm -1 on this change

As much as we would like to keep things up to date we can't make exceptions to our LTS or SemVer out of convenience.

There are no plans for new 6.x releases outside of a security release or major regression.

[gabrielschulhof]

@MylesBorins this is not really about keeping things up-to-date. It is about a promise we have made and broken. We claim that "N-API version 3" is available on all maintained versions of Node.js, but these APIs are not available on v6.x. Therefore we won't really have ABI stability until v6.x is no longer maintained.

The fact that the current minor release of v6.x claims to have N-API version 3 but doesn't is a bug. This is the fix. So, in that sense, this PR is not semver-minor. I would even argue that this is a critical fix because, without this fix, the whole ABI stability picture breaks down, to be restored only when v6.x becomes completely unsupported.

I admit that we might have planned our strategy of introducing new N-APIs as experimental sooner, in which case these APIs would also have landed under the experimental flag, and as such, would not have been a part of the N-API version 3 promise.

@nodejs/n-api it looks like one of the prerequisites of taking a new N-API out of experimental is that it be backported to all Node.js versions, so we never again run into this situation. In fact, we may have a very narrow window for getting N-APIs out of experimental because, once the oldest support Node.js goes into maintenance mode, we can not land the PR that modifies src/node_api.h to move the N-API out of the NAPI_EXPERIMENTAL section and under the version into which we wish to add it. So, the only window for moving N-APIs out of experimental is when the oldest Node.js is about to go into maintenance mode.

[addaleax]

An alternative would be to amend our LTS policy to account for a special status of N-API. One could definitely argue that it should have such a status, being an API that is explicitly not governed by semver itself, but rather stable indefinitely.

[gabrielschulhof]

@addaleax I imagine that even if N-API received such a concession, our choice of which APIs to mark as no-longer-experimental would be limited to those that were backported at the time that the oldest Node.js release went into Maintenance mode, because we would endeavour to always restrict PRs against such a release to re-shufflings of the order of things in src/node_api.h, rather than actual backporting. Still, having some leeway would be good.

ok with this landing as semver-patch

[MylesBorins]

Let's land this as semver-patch and consider it a bug fix. If the contract of n-api is consistency across all versions we should be suporting it as such. That being said I have mixed feelings about n-api having been declared as stable with changes like this landing afterwards... but this isn't the issue to debate that.

There are a couple of patches on 6.x-staging right now, including one that landed today for a breakage in BSD that we should get a release out for soon... let's land this and include it in an upcoming release.

Should we audit n-api and ensure that nothing else should land?

[gabrielschulhof]

@MylesBorins I'll have a look at the diff between master and this branch. There should be no more new APIs, because everything else should be behind NAPI_EXPERIMENTAL.

[gabrielschulhof]

@MylesBorins

diff -u \
  <(git show master:src/node_api.h | \
    sed -r 's/(napi_[a-zA-Z_]+)/\n\1\n/g' | \
    grep '^napi_' | sort -u) \
  <(git show backport-19377-to-v6.x:src/node_api.h | \
    sed -r 's/(napi_[a-zA-Z_]+)/\n\1\n/g' | \
    grep '^napi_' | sort -u) | \
less

shows only references to bigint and napi_threadsafe_function, both of which are experimental.

There are internal changes we could backport, like the fairly significant performance fix e41ccd4 but we need to decide if that's worth the churn.

[mhdawson]

+1 for landing, and if landing as a bug fix is the way to do that then sounds good to me.

[gabrielschulhof]

@MylesBorins should I backport the perf fix from e41ccd4?

[MylesBorins]

I'm apprehensive about backporting perf fixes but I defer to @mhdawson on this one

Perhaps, as suggested above, we should come up with a unique LTS policy specifically for n-api... as it working exactly the same on all branches is a huge part of the contract

[addaleax]

I would not backport e41ccd4 unless commits start to depend on it.

[mhdawson]

I agree with deferring the backport of e41ccd4

[richardlau]

Added Release-agenda label so this is picked up by the tooling for the next Release Working Group meeting.

[MylesBorins]

The original PR was semver-minor and we had decided for the 8.x napi PR to land it a similar change as semver-minor so I'm adding back the label.

Since 6.x is EOL in less than 3 months I think we are too late on doing this. As I mentioned a couple of months ago I would like to see us come up with an explicit process around n-api if we are going to want to land things in maintenance. My gut it we have missed the boat for 6.x, but I'm open to debate on this in the next release meeting

[mhdawson]

@MylesBorins are we planning a 6.x release for any other reasons. I agree that since we are so close to EOL the decision to land should be re-discussed in the next meeting. We might want to invite @gabrielschulhof so that he can be included in the discussion on the potential impact.

[BethGriggs]

@gabrielschulhof, this seems to be failing on osx in CI with the following error

12:07:39 ../src/env.h:20:10: fatal error: 'unordered_set' file not found
12:07:39 #include <unordered_set>
12:07:39          ^
12:07:39 In file included from ../src/cares_wrap.cc:3:
12:07:39 In file included from ../src/async-wrap-inl.h:7:
12:07:39 In file included from ../src/base-object-inl.h:7:
12:07:39 In file included from ../src/env-inl.h:6:
12:07:39 ../src/env.h:20:10: fatal error: 'unordered_set' file not found
12:07:39 #include <unordered_set>
12:07:39          ^
12:07:39 1 error generated.

https://ci.nodejs.org/job/node-test-commit-osx/25208/

[richardlau]

@gabrielschulhof, this seems to be failing on osx in CI with the following error

12:07:39 ../src/env.h:20:10: fatal error: 'unordered_set' file not found
12:07:39 #include <unordered_set>
12:07:39          ^
12:07:39 In file included from ../src/cares_wrap.cc:3:
12:07:39 In file included from ../src/async-wrap-inl.h:7:
12:07:39 In file included from ../src/base-object-inl.h:7:
12:07:39 In file included from ../src/env-inl.h:6:
12:07:39 ../src/env.h:20:10: fatal error: 'unordered_set' file not found
12:07:39 #include <unordered_set>
12:07:39          ^
12:07:39 1 error generated.

https://ci.nodejs.org/job/node-test-commit-osx/25208/

I suspect this is because v6.x is linked against libstdc++ on macOS (newer versions of Node.js link against libc++ but we can't change v6.x without risking ecosystem breakage: #24648 (comment)).

[addaleax]

Would it be okay to add #ifdef __APPLE__s to include <set> instead of macos + use #define unordered_set set? It’s not pretty but it should do the trick.

[gabrielschulhof]

@addaleax I made the change you suggested.

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

[gabrielschulhof]

@addaleax looks like there are more problems once we use set.

[gabrielschulhof]

@addaleax looks like we might need a later compiler than what we're using.

[addaleax]

@gireeshpunathil Yeah, sorry, I think the issue is that std::set is not a drop-in replacement for std::unordered_set, and we’d need to define a comparison class for that rather than hash + equal classes.

I’m happy to implement that if you want.

(Also, the perf difference between std::set and std::unordered_set probably isn’t all that important on v6.x – we use the cleanup hooks a lot more internally on the current release lines, but here we’d only care about externally added hooks.)

[gabrielschulhof]

@addaleax please, and thank you!