-
Notifications
You must be signed in to change notification settings - Fork 465
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 8 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 | ||
---|---|---|---|---|
|
@@ -179,8 +179,12 @@ namespace Napi { | |||
/// In the V8 JavaScript engine, a Node-API environment approximately | ||||
/// corresponds to an Isolate. | ||||
class Env { | ||||
private: | ||||
#if NAPI_VERSION > 2 | ||||
template <typename Hook, typename Arg = void> | ||||
class CleanupHook; | ||||
#endif // NAPI_VERSION > 2 | ||||
#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); | ||||
|
@@ -201,6 +205,14 @@ namespace Napi { | |||
Value RunScript(const std::string& utf8script); | ||||
Value RunScript(String script); | ||||
|
||||
#if NAPI_VERSION > 2 | ||||
template <typename Hook> | ||||
CleanupHook<Hook> AddCleanupHook(Hook hook); | ||||
|
||||
template <typename Hook, typename Arg> | ||||
CleanupHook<Hook, Arg> AddCleanupHook(Hook hook, Arg* arg); | ||||
#endif // NAPI_VERSION > 2 | ||||
|
||||
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(); | ||||
|
||||
|
@@ -219,7 +231,28 @@ namespace Napi { | |||
|
||||
private: | ||||
napi_env _env; | ||||
|
||||
#if NAPI_VERSION > 2 | ||||
template <typename Hook, typename Arg> | ||||
class CleanupHook { | ||||
public: | ||||
CleanupHook(Env env, Hook hook, Arg* arg); | ||||
CleanupHook(Env env, Hook hook); | ||||
bool Remove(Env env); | ||||
bool IsEmpty() const; | ||||
|
||||
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; | ||||
}; | ||||
}; | ||||
#endif // NAPI_VERSION > 2 | ||||
|
||||
/// A JavaScript value of unknown type. | ||||
/// | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
#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); | ||
} | ||
|
||
int added = 0; | ||
|
||
added += !hook1.IsEmpty(); | ||
added += !hook1b.IsEmpty(); | ||
added += !hook2.IsEmpty(); | ||
added += !hook3.IsEmpty(); | ||
added += !hook3b.IsEmpty(); | ||
added += !hook4.IsEmpty(); | ||
added += !hook5.IsEmpty(); | ||
added += !hook6.IsEmpty(); | ||
|
||
return Number::New(env, added); | ||
} | ||
|
||
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,57 @@ | ||
'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); | ||
const actualAdded = binding.env_cleanup.addHooks(remove_hooks); | ||
const expectedAdded = remove_hooks === true ? 0 : 8; | ||
assert(actualAdded === expectedAdded, 'Incorrect number of hooks added'); | ||
} | ||
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') | ||
} | ||
} | ||
} | ||
|
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.
Could this version just call the other one and pass nullptr for data ?
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.
@mhdawson no because each constructor initializes the instance member
wrapper
to a different value,Env::CleanupHook<Hook, Arg>::Wrapper
(this one) orEnv::CleanupHook<Hook, Arg>::WrapperWithArg
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.
Thanks, nervermind.