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

src: add Env::AddCleanupHook #1014

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions doc/env.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,41 @@ Associates a data item stored at `T* data` with the current instance of the
addon. The item will be passed to the function `fini` which gets called when an
instance of the addon is unloaded. This overload accepts an additional hint to
be passed to `fini`.

### AddCleanupHook

```cpp
template <typename Hook>
CleanupHook<Hook> AddCleanupHook(Hook hook);
```

- `[in] hook`: A function to call when the environment exists. Accepts a
function of the form `void ()`.

Registers `hook` as a function to be run once the current Node.js environment
exits. Unlike the underlying C-based Node-API, providing the same `hook`
multiple times **is** allowed. The hooks will be called in reverse order, i.e.
the most recently added one will be called first.

Returns an `Env::CleanupHook` object, which can be used to remove the hook via
its `Remove()` method.

### AddCleanupHook

```cpp
template <typename Hook, typename Arg>
CleanupHook<Hook, Arg> AddCleanupHook(Hook hook, Arg* arg);
```

- `[in] hook`: A function to call when the environment exists. Accepts a
function of the form `void (Arg* arg)`.
- `[in] arg`: A pointer to data that will be passed as the argument to `hook`.

Registers `hook` as a function to be run with the `arg` parameter once the
current Node.js environment exits. Unlike the underlying C-based Node-API,
providing the same `hook` and `arg` pair multiple times **is** allowed. The
hooks will be called in reverse order, i.e. the most recently added one will be
called first.

Returns an `Env::CleanupHook` object, which can be used to remove the hook via
its `Remove()` method.
57 changes: 57 additions & 0 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,23 @@ inline Value Env::RunScript(String script) {
return Value(_env, result);
}

template <typename Hook, typename Arg>
void Env::CleanupHook<Hook, Arg>::Wrapper(void* data) NAPI_NOEXCEPT {
auto* cleanupData =
static_cast<typename Napi::Env::CleanupHook<Hook, Arg>::CleanupData*>(
data);
cleanupData->hook();
delete cleanupData;
}
template <typename Hook, typename Arg>
void Env::CleanupHook<Hook, Arg>::WrapperWithArg(void* data) NAPI_NOEXCEPT {
auto* cleanupData =
static_cast<typename Napi::Env::CleanupHook<Hook, Arg>::CleanupData*>(
data);
cleanupData->hook(static_cast<Arg*>(cleanupData->arg));
delete cleanupData;
}

#if NAPI_VERSION > 5
template <typename T, Env::Finalizer<T> fini>
inline void Env::SetInstanceData(T* data) {
Expand Down Expand Up @@ -5692,6 +5709,46 @@ Addon<T>::DefineProperties(Object object,
}
#endif // NAPI_VERSION > 5

template <typename Hook, typename Arg>
Env::CleanupHook<Hook, Arg> Env::AddCleanupHook(Hook hook, Arg* arg) {
return CleanupHook<Hook, Arg>(*this, hook, arg);
}

template <typename Hook>
Env::CleanupHook<Hook> Env::AddCleanupHook(Hook hook) {
return CleanupHook<Hook>(*this, hook);
}

template <typename Hook, typename Arg>
Env::CleanupHook<Hook, Arg>::CleanupHook(Napi::Env env, Hook hook)
: wrapper(Env::CleanupHook<Hook, Arg>::Wrapper) {
data = new CleanupData{std::move(hook), nullptr};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this version just call the other one and pass nullptr for data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson no because each constructor initializes the instance member wrapper to a different value, Env::CleanupHook<Hook, Arg>::Wrapper (this one) or Env::CleanupHook<Hook, Arg>::WrapperWithArg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nervermind.

napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
NAPI_FATAL_IF_FAILED(status,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would cause napi_add_env_cleanup_hook to fail ? I'm wondering if we can let the caller trying add a hook know it failed versus causing a FATAL error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the implementation, it looks like it should always return napi_ok ... https://github.com/nodejs/node/blob/663d7e9fb27f017f38b79382a95fec10e5f66f9a/src/node_api.cc#L656-L676

According to the docs it can just abort the process with invalid arguments:

Providing the same fun and arg values multiple times is not allowed and will lead the process to abort.

But we would never provide the same arg value (as we always create a new CallbackData)

Maybe we don't need the check at all...? However napi_add_env_cleanup_hook always returning napi_ok is an implementation detail, maybe we should keep it blackboxed 🤷

How would you recommend "add a hook know it failed" ...?

Thanks, Kevin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you recommend "add a hook know it failed" . What I had in mind was some kind of return value the caller could check. For example if it was documented that the callers should check that the result from AddCleanupHook was non-null would that make any sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. Added a bool IsEmpty() method on Env::CleanupHook

node-addon-api/napi-inl.h

Lines 5756 to 5759 in 36ef600

template <class Hook, class Arg>
bool Env::CleanupHook<Hook, Arg>::IsEmpty() const {
return data == nullptr;
}

data is nullptr if adding the hook failed:

node-addon-api/napi-inl.h

Lines 5737 to 5746 in 36ef600

template <typename Hook, typename Arg>
Env::CleanupHook<Hook, Arg>::CleanupHook(Napi::Env env, Hook hook, Arg* arg)
: wrapper(Env::CleanupHook<Hook, Arg>::WrapperWithArg) {
data = new CleanupData{std::move(hook), arg};
napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
if (status != napi_ok) {
delete data;
data = nullptr;
}
}

"Env::CleanupHook<Hook, Arg>::CleanupHook Wrapper",
"napi_add_env_cleanup_hook");
}

template <typename Hook, typename Arg>
Env::CleanupHook<Hook, Arg>::CleanupHook(Napi::Env env, Hook hook, Arg* arg)
: wrapper(Env::CleanupHook<Hook, Arg>::WrapperWithArg) {
data = new CleanupData{std::move(hook), arg};
napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
NAPI_FATAL_IF_FAILED(
status,
"Env::CleanupHook<Hook, Arg>::CleanupHook WrapperWithArg",
"napi_add_env_cleanup_hook");
}

template <class Hook, class Arg>
void Env::CleanupHook<Hook, Arg>::Remove(Env env) {
napi_status status = napi_remove_env_cleanup_hook(env, wrapper, data);
NAPI_FATAL_IF_FAILED(status,
"Env::CleanupHook<Hook, Arg>::Remove",
"napi_remove_env_cleanup_hook");
delete data;
}

} // namespace Napi

#endif // SRC_NAPI_INL_H_
28 changes: 27 additions & 1 deletion napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,10 @@ namespace Napi {
/// In the V8 JavaScript engine, a Node-API environment approximately
/// corresponds to an Isolate.
class Env {
private:
template <typename Hook, typename Arg = void>
class CleanupHook;
#if NAPI_VERSION > 5
private:
template <typename T> static void DefaultFini(Env, T* data);
template <typename DataType, typename HintType>
static void DefaultFiniWithHint(Env, DataType* data, HintType* hint);
Expand All @@ -201,6 +203,12 @@ namespace Napi {
Value RunScript(const std::string& utf8script);
Value RunScript(String script);

template <typename Hook>
CleanupHook<Hook> AddCleanupHook(Hook hook);

template <typename Hook, typename Arg>
CleanupHook<Hook, Arg> AddCleanupHook(Hook hook, Arg* arg);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these need to be guarded by NAPI_VERSION > 3 as the cleanup hooks were only added in version 3 as per: https://nodejs.org/api/n-api.html#n_api_napi_add_env_cleanup_hook

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but we dropped support for node 8, and Node-API version is present in 10+. Do we still need a guard?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, people can still ask for a particular Node-API version and I think we should respect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I did not add guards to the tests -- we will always test Node-API 3+ right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of guarding the tests its only a 1 line addition here:

if (napiVersion < 3) {
so I think we might as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more I guess there is more to it than that. You also need to guard the compilation but that is pretty easy too. Unlikely anybody will test with napiVersion <3 but for consistency I'd still prefer if we guard since it's not a lot of effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

#if NAPI_VERSION > 5
template <typename T> T* GetInstanceData();

Expand All @@ -219,6 +227,24 @@ namespace Napi {

private:
napi_env _env;

template <typename Hook, typename Arg>
class CleanupHook {
public:
CleanupHook(Env env, Hook hook, Arg* arg);
CleanupHook(Env env, Hook hook);
void Remove(Env env);

private:
static inline void Wrapper(void* data) NAPI_NOEXCEPT;
static inline void WrapperWithArg(void* data) NAPI_NOEXCEPT;

void (*wrapper)(void* arg);
struct CleanupData {
Hook hook;
Arg* arg;
} * data;
};
};

/// A JavaScript value of unknown type.
Expand Down
2 changes: 2 additions & 0 deletions test/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Object InitDate(Env env);
#endif
Object InitDataView(Env env);
Object InitDataViewReadWrite(Env env);
Object InitEnvCleanup(Env env);
Object InitError(Env env);
Object InitExternal(Env env);
Object InitFunction(Env env);
Expand Down Expand Up @@ -103,6 +104,7 @@ Object Init(Env env, Object exports) {
exports.Set("dataview", InitDataView(env));
exports.Set("dataview_read_write", InitDataView(env));
exports.Set("dataview_read_write", InitDataViewReadWrite(env));
exports.Set("env_cleanup", InitEnvCleanup(env));
exports.Set("error", InitError(env));
exports.Set("external", InitExternal(env));
exports.Set("function", InitFunction(env));
Expand Down
1 change: 1 addition & 0 deletions test/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'callbackscope.cc',
'dataview/dataview.cc',
'dataview/dataview_read_write.cc',
'env_cleanup.cc',
'error.cc',
'external.cc',
'function.cc',
Expand Down
70 changes: 70 additions & 0 deletions test/env_cleanup.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#include <stdio.h>
#include "napi.h"

using namespace Napi;

static void cleanup(void* arg) {
printf("static cleanup(%d)\n", *(int*)(arg));
}
static void cleanupInt(int* arg) {
printf("static cleanup(%d)\n", *(arg));
}

static void cleanupVoid() {
printf("static cleanup()\n");
}

static int secret1 = 42;
static int secret2 = 43;

Value AddHooks(const CallbackInfo& info) {
auto env = info.Env();

bool shouldRemove = info[0].As<Boolean>().Value();

// hook: void (*)(void *arg), hint: int
auto hook1 = env.AddCleanupHook(cleanup, &secret1);
// test using same hook+arg pair
auto hook1b = env.AddCleanupHook(cleanup, &secret1);

// hook: void (*)(int *arg), hint: int
auto hook2 = env.AddCleanupHook(cleanupInt, &secret2);

// hook: void (*)(int *arg), hint: void (default)
auto hook3 = env.AddCleanupHook(cleanupVoid);
// test using the same hook
auto hook3b = env.AddCleanupHook(cleanupVoid);

// hook: lambda []void (int *arg)->void, hint: int
auto hook4 = env.AddCleanupHook(
[&](int* arg) { printf("lambda cleanup(%d)\n", *arg); }, &secret1);

// hook: lambda []void (void *)->void, hint: void
auto hook5 =
env.AddCleanupHook([&](void*) { printf("lambda cleanup(void)\n"); },
static_cast<void*>(nullptr));

// hook: lambda []void ()->void, hint: void (default)
auto hook6 = env.AddCleanupHook([&]() { printf("lambda cleanup()\n"); });

if (shouldRemove) {
hook1.Remove(env);
hook1b.Remove(env);
hook2.Remove(env);
hook3.Remove(env);
hook3b.Remove(env);
hook4.Remove(env);
hook5.Remove(env);
hook6.Remove(env);
}

return env.Undefined();
}

Object InitEnvCleanup(Env env) {
Object exports = Object::New(env);

exports["addHooks"] = Function::New(env, AddHooks);

return exports;
}
55 changes: 55 additions & 0 deletions test/env_cleanup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';

const assert = require('assert');

if (process.argv[2] === 'runInChildProcess') {
const binding_path = process.argv[3];
const remove_hooks = process.argv[4] === 'true';

const binding = require(binding_path);
binding.env_cleanup.addHooks(remove_hooks);
}
else {
module.exports = require('./common').runTestWithBindingPath(test);
}

function test(bindingPath) {
for (const remove_hooks of [false, true]) {
const { status, output } = require('./napi_child').spawnSync(
process.execPath,
[
__filename,
'runInChildProcess',
bindingPath,
remove_hooks,
],
{ encoding: 'utf8' }
);

const stdout = output[1].trim();
/**
* There is no need to sort the lines, as per Node-API documentation:
* > The hooks will be called in reverse order, i.e. the most recently
* > added one will be called first.
*/
const lines = stdout.split(/[\r\n]+/);

assert(status === 0, `Process aborted with status ${status}`);

if (remove_hooks) {
assert.deepStrictEqual(lines, [''], 'Child process had console output when none expected')
} else {
assert.deepStrictEqual(lines, [
'lambda cleanup()',
'lambda cleanup(void)',
'lambda cleanup(42)',
'static cleanup()',
'static cleanup()',
'static cleanup(43)',
'static cleanup(42)',
'static cleanup(42)'
], 'Child process console output mismisatch')
}
}
}