src: add Env::AddCleanupHook by KevinEady · Pull Request #1014 · nodejs/node-addon-api

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

Adds wrapping for napi_add_env_cleanup_hook and napi_remove_env_cleanup_hook.

wip

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

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

+    : 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

+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 <midawson@redhat.com>

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

Landed as 10564a4

Add CleanupHook support to Env PR-URL: nodejs#1014 Reviewed-By: Michael Dawson <midawson@redhat.com>

Add CleanupHook support to Env PR-URL: nodejs/node-addon-api#1014 Reviewed-By: Michael Dawson <midawson@redhat.com>

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

Comment thread napi.h Outdated

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 <midawson@redhat.com>

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 <midawson@redhat.com>

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 <midawson@redhat.com>

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 <midawson@redhat.com>

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 <midawson@redhat.com>

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 <midawson@redhat.com>

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 <midawson@redhat.com>


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

Uh oh!

Conversation

KevinEady commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KevinEady commented Jun 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KevinEady commented Jun 25, 2021 •

edited

Loading

KevinEady commented Jun 26, 2021 •

edited

Loading