-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: clean up resources on Environment teardown #19377
Changes from all commits
292b53a
ca372b3
16e7a35
9aa3c8f
9f0e5b1
ef4779e
89697e7
2738964
7e4114d
291d97c
2046851
18ee5b9
8db8cc8
32e3770
8bed0af
2aab562
b1110a0
204792f
fae778b
e355d11
29e0584
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 |
---|---|---|
|
@@ -886,6 +886,58 @@ If still valid, this API returns the `napi_value` representing the | |
JavaScript `Object` associated with the `napi_ref`. Otherwise, result | ||
will be NULL. | ||
|
||
### Cleanup on exit of the current Node.js instance | ||
|
||
While a Node.js process typically releases all its resources when exiting, | ||
embedders of Node.js, or future Worker support, may require addons to register | ||
clean-up hooks that will be run once the current Node.js instance exits. | ||
|
||
N-API provides functions for registering and un-registering such callbacks. | ||
When those callbacks are run, all resources that are being held by the addon | ||
should be freed up. | ||
|
||
#### napi_add_env_cleanup_hook | ||
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
```C | ||
NODE_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, | ||
void (*fun)(void* arg), | ||
void* arg); | ||
``` | ||
|
||
Registers `fun` as a function to be run with the `arg` parameter once the | ||
current Node.js environment exits. | ||
|
||
A function can safely be specified multiple times with different | ||
`arg` values. In that case, it will be called multiple times as well. | ||
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. Probably worth specifying in which order the hooks run. |
||
Providing the same `fun` and `arg` values multiple times is not allowed | ||
and will lead the process to abort. | ||
|
||
The hooks will be called in reverse order, i.e. the most recently added one | ||
will be called first. | ||
|
||
Removing this hook can be done by using `napi_remove_env_cleanup_hook`. | ||
Typically, that happens when the resource for which this hook was added | ||
is being torn down anyway. | ||
|
||
#### napi_remove_env_cleanup_hook | ||
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
```C | ||
NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, | ||
void (*fun)(void* arg), | ||
void* arg); | ||
``` | ||
|
||
Unregisters `fun` as a function to be run with the `arg` parameter once the | ||
current Node.js environment exits. Both the argument and the function value | ||
need to be exact matches. | ||
|
||
The function must have originally been registered | ||
with `napi_add_env_cleanup_hook`, otherwise the process will abort. | ||
|
||
## Module registration | ||
N-API modules are registered in a manner similar to other modules | ||
except that instead of using the `NODE_MODULE` macro the following | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -349,8 +349,35 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, | |
handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg}); | ||
} | ||
|
||
inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { | ||
handle_cleanup_waiting_--; | ||
template <typename T, typename OnCloseCallback> | ||
inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { | ||
handle_cleanup_waiting_++; | ||
static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle"); | ||
static_assert(offsetof(T, data) == offsetof(uv_handle_t, data), | ||
"T is a libuv handle"); | ||
static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb), | ||
"T is a libuv handle"); | ||
struct CloseData { | ||
Environment* env; | ||
OnCloseCallback callback; | ||
void* original_data; | ||
}; | ||
handle->data = new CloseData { this, callback, handle->data }; | ||
uv_close(reinterpret_cast<uv_handle_t*>(handle), [](uv_handle_t* handle) { | ||
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. Question: why not use a // This struct is used to close over these three values, and is passed back tough the handle->data pointer.
// It is stored on the heap and is freed by std::unique_ptr I would suggest using a different name for the 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.
Because that doesn’t work, it’s not convertible to the C signature that libuv needs 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. Ack. 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. So will you be willing to add a comment, since it's a bit of non trivial code + your last comment that a 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. @refack Yeah, sorry, should have been clearer about that – I’ll add a comment. :) Fwiw, we’re also running into this issue in a lot of places in core – capturing C++ lambdas and the libuv API just don’t mix very well. I think it’s fine to leave some short comments, but ultimately, it’s just a language-level thing… |
||
std::unique_ptr<CloseData> data { static_cast<CloseData*>(handle->data) }; | ||
data->env->handle_cleanup_waiting_--; | ||
handle->data = data->original_data; | ||
data->callback(reinterpret_cast<T*>(handle)); | ||
}); | ||
} | ||
|
||
void Environment::IncreaseWaitingRequestCounter() { | ||
request_waiting_++; | ||
} | ||
|
||
void Environment::DecreaseWaitingRequestCounter() { | ||
request_waiting_--; | ||
CHECK_GE(request_waiting_, 0); | ||
} | ||
|
||
inline uv_loop_t* Environment::event_loop() const { | ||
|
@@ -532,6 +559,14 @@ void Environment::SetUnrefImmediate(native_immediate_callback cb, | |
CreateImmediate(cb, data, obj, false); | ||
} | ||
|
||
inline bool Environment::can_call_into_js() const { | ||
return can_call_into_js_; | ||
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 technically, we cannot call into JS whenever there is a pending exception? Is is possible for someone to bump into a pending exception while this returns true? 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 would expect that, yes. But the code should be laid out to handle such a situation anyway, right? The function isn’t meant to give an exhausting result that tells code whether it’s okay to call into JS or not… it’s more of an “allowed to” than “able to”. Maybe we should rename it to 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. @addaleax In my understanding 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. Or, if we don't want to be too board about the meaning of this...maybe just something like 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.
Sure, I’m adding a comment. :)
I think I’d prefer to name it according to how its consumers use it. I don’t know why we would, but I wouldn’t consider it impossible that at some point we may want to use this flag for other things too. |
||
} | ||
|
||
inline void Environment::set_can_call_into_js(bool can_call_into_js) { | ||
can_call_into_js_ = can_call_into_js; | ||
} | ||
|
||
inline performance::performance_state* Environment::performance_state() { | ||
return performance_state_.get(); | ||
} | ||
|
@@ -629,6 +664,29 @@ inline void Environment::SetTemplateMethod(v8::Local<v8::FunctionTemplate> that, | |
t->SetClassName(name_string); // NODE_SET_METHOD() compatibility. | ||
} | ||
|
||
void Environment::AddCleanupHook(void (*fn)(void*), void* arg) { | ||
auto insertion_info = cleanup_hooks_.emplace(CleanupHookCallback { | ||
fn, arg, cleanup_hook_counter_++ | ||
}); | ||
// Make sure there was no existing element with these values. | ||
CHECK_EQ(insertion_info.second, true); | ||
} | ||
|
||
void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { | ||
CleanupHookCallback search { fn, arg, 0 }; | ||
cleanup_hooks_.erase(search); | ||
} | ||
|
||
size_t Environment::CleanupHookCallback::Hash::operator()( | ||
const CleanupHookCallback& cb) const { | ||
return std::hash<void*>()(cb.arg_); | ||
} | ||
|
||
bool Environment::CleanupHookCallback::Equal::operator()( | ||
const CleanupHookCallback& a, const CleanupHookCallback& b) const { | ||
return a.fn_ == b.fn_ && a.arg_ == b.arg_; | ||
} | ||
|
||
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName) | ||
#define VS(PropertyName, StringValue) V(v8::String, PropertyName) | ||
#define V(TypeName, PropertyName) \ | ||
|
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.
one question: if I call
napi_add_env_cleanup_hook
multiple times with the same function and arg, how can I just remove one of them when callingnapi_remove_env_cleanup_hook
?Edit: I saw unorder_set::emplace() and erase(). In my case, it only add one callback into the cleanup hook.
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.
@yhwang You can’t do it, right now it would crash with that – I’ve clarified this in the documentation.
The reason is that you basically never want that. If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state; in that case, your code will probably not work anyway in the scenarios that this is meant to support.
(Another reason is the suspicion – not benchmarked – that a
std::unordered_set
is more performant than astd::unordered_multiset
.)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.
hmm, the
arg
could be a refcount object and decrease its counter in each call of the callback function :-pI saw you said
with different arg values
now. Good catch!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.
Yeah, you could still get that kind of behaviour by creating pointers with an additional layer of indirection if you really want to and using those as unique keys. But like I said, probably not something one really wants to do :)