src: add Env::AddCleanupHook #1014

KevinEady · 2021-06-25T01:16:17Z

Adds wrapping for napi_add_env_cleanup_hook and napi_remove_env_cleanup_hook.

wip

napi.h

KevinEady · 2021-06-26T10:11:43Z

Hi team. This is ready for review. I have fixed the API privatization, see https://github.com/nodejs/node-addon-api/pull/1014/files#diff-6294f8ee1d8cd7e4e94cfc22a083380fb80058e34f53100d895b9b3b295dd801

Env::CleanupHook is a private class in Env with public constructors, so only Env can construct an instance and eg. return it to the caller in Env::AddCleanupHook.
Env::CleanupHook exposes one public method, Remove.
Env::CleanupHook has the two private static wrapper methods (one for calling hook() and one for hook(arg)). They are referenced inside the constructor.

mhdawson · 2021-06-28T20:45:59Z

napi.h

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


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

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?

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

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

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

node-addon-api/test/index.js

Line 84 in 6697c51

if (napiVersion < 3) {

so I think we might as well.

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.

mhdawson · 2021-06-28T20:51:01Z

napi-inl.h

+    : wrapper(Env::CleanupHook<Hook, Arg>::Wrapper) {
+  data = new CleanupData{std::move(hook), nullptr};
+  napi_status status = napi_add_env_cleanup_hook(env, wrapper, data);
+  NAPI_FATAL_IF_FAILED(status,


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.

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

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?

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;

}

}

- add `NAPI_VERSION` guard - add `IsEmpty`

mhdawson · 2021-07-16T18:07:16Z

napi-inl.h

+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};


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

@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

Thanks, nervermind.

mhdawson

LGTM

Add CleanupHook support to Env PR-URL: #1014 Reviewed-By: Michael Dawson <[email protected]>

mhdawson · 2021-07-27T15:00:45Z

Landed as 10564a4

Add CleanupHook support to Env PR-URL: nodejs#1014 Reviewed-By: Michael Dawson <[email protected]>

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <[email protected]>

KevinEady added 2 commits June 25, 2021 00:37

src: add AddCleanupHook

ef9bc1b

wip

some refactor

23ad21e

KevinEady commented Jun 25, 2021

View reviewed changes

napi.h Outdated Show resolved Hide resolved

KevinEady added 3 commits June 25, 2021 22:30

cleanup private implementation; fix tests

370e742

rename Hint to Arg; better class privatization

cca2da6

add docs; update tests

42968bc

KevinEady force-pushed the add-env-cleanup-hook branch from 4ee44c1 to 42968bc Compare June 26, 2021 09:31

KevinEady marked this pull request as ready for review June 26, 2021 10:01

fix node-api version guard

f4ce9e3

mhdawson reviewed Jun 28, 2021

View reviewed changes

address review comments

36ef600

- add `NAPI_VERSION` guard - add `IsEmpty`

KevinEady force-pushed the add-env-cleanup-hook branch from 36f13d3 to 36ef600 Compare July 16, 2021 14:47

Fix guards

ddee245

mhdawson reviewed Jul 16, 2021

View reviewed changes

add guards to tests

7d2828f

mhdawson approved these changes Jul 23, 2021

View reviewed changes

mhdawson pushed a commit that referenced this pull request Jul 27, 2021

src: add AddCleanupHook

10564a4

Add CleanupHook support to Env PR-URL: #1014 Reviewed-By: Michael Dawson <[email protected]>

mhdawson closed this Jul 27, 2021

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021

src: add AddCleanupHook

daf910e

Add CleanupHook support to Env PR-URL: nodejs#1014 Reviewed-By: Michael Dawson <[email protected]>

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021

src: add AddCleanupHook

fe8de63

Add CleanupHook support to Env PR-URL: nodejs#1014 Reviewed-By: Michael Dawson <[email protected]>

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022

src: add AddCleanupHook

a435032

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <[email protected]>

Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022

src: add AddCleanupHook

53a133a

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <[email protected]>

wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022

src: add AddCleanupHook

39d9603

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <[email protected]>

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023

src: add AddCleanupHook

6eb12cc

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add Env::AddCleanupHook #1014

src: add Env::AddCleanupHook #1014

KevinEady commented Jun 25, 2021 •

edited

Loading

KevinEady commented Jun 26, 2021 •

edited

Loading

mhdawson Jun 28, 2021

KevinEady Jun 29, 2021

mhdawson Jun 29, 2021

KevinEady Jul 16, 2021

KevinEady Jul 16, 2021

mhdawson Jul 16, 2021

mhdawson Jul 16, 2021

KevinEady Jul 23, 2021

mhdawson Jun 28, 2021

KevinEady Jun 29, 2021

mhdawson Jun 29, 2021

KevinEady Jul 16, 2021

mhdawson Jul 16, 2021

KevinEady Jul 23, 2021

mhdawson Jul 23, 2021

mhdawson left a comment

mhdawson commented Jul 27, 2021


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

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

	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;
	}
	}

src: add Env::AddCleanupHook #1014

src: add Env::AddCleanupHook #1014

Conversation

KevinEady commented Jun 25, 2021 • edited Loading

KevinEady commented Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 27, 2021

KevinEady commented Jun 25, 2021 •

edited

Loading

KevinEady commented Jun 26, 2021 •

edited

Loading