A recent change, 215027c8ed, introduced flakiness into our
test suite that exposed an issue with the cleanup hook API design.
Specifically, the signatures of `AddEnvironmentCleanupHook()` and
`RemoveEnvironmentCleanupHook()` are problematic. Both functions
take `Isolate*` arguments, as addons are not generally expected
to have to care about the Node.js `Environment` as a first-class
scope provider.
However, this model made the incorrect assumption that in the
situations in which `RemoveEnvironmentCleanupHook()` would be
invoked an `Environment` would always be associated with the
current `Isolate` (via the current V8 `Context`, if there is one).
This occasionally breaks down when `RemoveEnvironmentCleanupHook()`
is called during garbage collection -- which would be an expected
use case of the functionality, but one that has not been covered
through our tests before 215027c8ed.
Since Node.js guarantees API and ABI stability within a major version,
and this is a bug that is independent from the aforementioned change,
this commit resolves it by adding global mutable state to keep track
off cleanup hooks registered through the Node.js public API.
Obviously, this solution does not represent a desirable long-term
state, and a semver-minor follow up should add an API that does not
require modifications to these data structures, likely based on
the async cleanup hook API which already solves this issue properly.
Refs: https://github.com/nodejs/node/pull/63642
Fixes: https://github.com/nodejs/node/issues/63923
Signed-off-by: Anna Henningsen <anna@addaleax.net>
PR-URL: https://github.com/nodejs/node/pull/63985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1723773d41