-
Notifications
You must be signed in to change notification settings - Fork 464
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
Changes from 2 commits
ef9bc1b
23ad21e
370e742
cca2da6
42968bc
f4ce9e3
36ef600
ddee245
7d2828f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -135,6 +135,8 @@ namespace Napi { | |||
class CallbackInfo; | ||||
class TypedArray; | ||||
template <typename T> class TypedArrayOf; | ||||
template <typename Hook, typename Hint = void> | ||||
class CleanupHook; | ||||
|
||||
using Int8Array = | ||||
TypedArrayOf<int8_t>; ///< Typed-array of signed 8-bit integers | ||||
|
@@ -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 Hint> | ||||
CleanupHook<Hook, Hint> AddCleanupHook(Hook hook, Hint* hint); | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: Line 84 in 6697c51
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed. |
||||
#if NAPI_VERSION > 5 | ||||
template <typename T> T* GetInstanceData(); | ||||
|
||||
|
@@ -221,6 +229,21 @@ namespace Napi { | |||
napi_env _env; | ||||
}; | ||||
|
||||
template <typename Hook, typename Hint> | ||||
class CleanupHook { | ||||
public: | ||||
struct CleanupData { | ||||
KevinEady marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
Hook hook; | ||||
Hint* hint; | ||||
}; | ||||
CleanupHook(Env env, void (*wrapper)(void* arg), Hook hook, Hint* hint); | ||||
void Remove(Env env); | ||||
|
||||
private: | ||||
void (*wrapper)(void* arg); | ||||
CleanupData* data; | ||||
}; | ||||
|
||||
/// A JavaScript value of unknown type. | ||||
/// | ||||
/// For type-specific operations, convert to one of the Value subclasses using a `To*` or `As()` | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
#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); | ||
|
||
// hook: void (*)(int *arg), hint: int | ||
auto hook2 = env.AddCleanupHook(cleanupInt, &secret2); | ||
|
||
// hook: void (*)(int *arg), hint: void | ||
auto hook3 = 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); | ||
hook2.Remove(env); | ||
hook3.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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// @ts-check | ||
|
||
'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(); | ||
const lines = stdout.split(/\n/).sort(); | ||
|
||
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(42)", | ||
"lambda cleanup(void)", | ||
"static cleanup()", | ||
"static cleanup(42)", | ||
"static cleanup(43)", | ||
], 'Child process console output mismisatch') | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-L676According to the docs it can just abort the process with invalid arguments:
But we would never provide the same
arg
value (as we always create anew CallbackData
)Maybe we don't need the check at all...? However
napi_add_env_cleanup_hook
always returningnapi_ok
is an implementation detail, maybe we should keep it blackboxed 🤷How would you recommend "add a hook know it failed" ...?
Thanks, Kevin
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 onEnv::CleanupHook
node-addon-api/napi-inl.h
Lines 5756 to 5759 in 36ef600
data
is nullptr if adding the hook failed:node-addon-api/napi-inl.h
Lines 5737 to 5746 in 36ef600