Skip to content
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

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@yhwang yhwang Mar 16, 2018

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 calling napi_remove_env_cleanup_hook?

Edit: I saw unorder_set::emplace() and erase(). In my case, it only add one callback into the cleanup hook.

Copy link
Member Author

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 a std::unordered_multiset.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state

hmm, the arg could be a refcount object and decrease its counter in each call of the callback function :-p

I saw you said with different arg values now. Good catch!

Copy link
Member Author

@addaleax addaleax Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass the same arg + fun combination twice in a meaningful way, that means that you rely on global state

hmm, the arg could be a refcount object and decrease its counter in each call of the callback function :-p

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

`arg` values. In that case, it will be called multiple times as well.
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
25 changes: 17 additions & 8 deletions lib/internal/process/stdio.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,15 @@ function setupStdio() {
stdin = new net.Socket({
handle: process.channel,
readable: true,
writable: false
writable: false,
manualStart: true
});
} else {
stdin = new net.Socket({
fd: fd,
readable: true,
writable: false
writable: false,
manualStart: true
});
}
// Make sure the stdin can't be `.end()`-ed
Expand All @@ -107,15 +109,22 @@ function setupStdio() {
stdin._handle.readStop();
}

// if the user calls stdin.pause(), then we need to stop reading
// immediately, so that the process can close down.
// If the user calls stdin.pause(), then we need to stop reading
// once the stream implementation does so (one nextTick later),
// so that the process can close down.
stdin.on('pause', () => {
process.nextTick(onpause);
});

function onpause() {
if (!stdin._handle)
return;
stdin._readableState.reading = false;
stdin._handle.reading = false;
stdin._handle.readStop();
});
if (stdin._handle.reading && !stdin._readableState.flowing) {
stdin._readableState.reading = false;
stdin._handle.reading = false;
stdin._handle.readStop();
}
}

return stdin;
}
Expand Down
7 changes: 5 additions & 2 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) {
do {
std::vector<double> destroy_async_id_list;
destroy_async_id_list.swap(*env->destroy_async_id_list());
if (!env->can_call_into_js()) return;
for (auto async_id : destroy_async_id_list) {
// Want each callback to be cleaned up after itself, instead of cleaning
// them all up after the while() loop completes.
Expand All @@ -166,7 +167,7 @@ void Emit(Environment* env, double async_id, AsyncHooks::Fields type,
Local<Function> fn) {
AsyncHooks* async_hooks = env->async_hooks();

if (async_hooks->fields()[type] == 0)
if (async_hooks->fields()[type] == 0 || !env->can_call_into_js())
return;

v8::HandleScope handle_scope(env->isolate());
Expand Down Expand Up @@ -625,8 +626,10 @@ void AsyncWrap::EmitTraceEventDestroy() {
}

void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0)
if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0 ||
!env->can_call_into_js()) {
return;
}

if (env->destroy_async_id_list()->empty()) {
env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr);
Expand Down
9 changes: 9 additions & 0 deletions src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
CHECK_EQ(false, object.IsEmpty());
CHECK_GT(object->InternalFieldCount(), 0);
object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
}


BaseObject::~BaseObject() {
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));

if (persistent_handle_.IsEmpty()) {
// This most likely happened because the weak callback below cleared it.
return;
Expand Down Expand Up @@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
}


void BaseObject::DeleteMe(void* data) {
BaseObject* self = static_cast<BaseObject*>(data);
delete self;
}


void BaseObject::MakeWeak() {
persistent_handle_.SetWeak(
this,
Expand Down
2 changes: 2 additions & 0 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ class BaseObject {
private:
BaseObject();

static inline void DeleteMe(void* data);

// persistent_handle_ needs to be at a fixed offset from the start of the
// class because it is used by src/node_postmortem_metadata.cc to calculate
// offsets and generate debug symbols for BaseObject, which assumes that the
Expand Down
40 changes: 15 additions & 25 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) {
}


void ares_poll_close_cb(uv_handle_t* watcher) {
node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher,
reinterpret_cast<uv_poll_t*>(watcher));
void ares_poll_close_cb(uv_poll_t* watcher) {
node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher);
free(task);
}

Expand Down Expand Up @@ -347,8 +346,7 @@ void ares_sockstate_cb(void* data,
"When an ares socket is closed we should have a handle for it");

channel->task_list()->erase(it);
uv_close(reinterpret_cast<uv_handle_t*>(&task->poll_watcher),
ares_poll_close_cb);
channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb);

if (channel->task_list()->empty()) {
uv_timer_stop(channel->timer_handle());
Expand Down Expand Up @@ -517,10 +515,7 @@ ChannelWrap::~ChannelWrap() {
void ChannelWrap::CleanupTimer() {
if (timer_handle_ == nullptr) return;

uv_close(reinterpret_cast<uv_handle_t*>(timer_handle_),
[](uv_handle_t* handle) {
delete reinterpret_cast<uv_timer_t*>(handle);
});
env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; });
timer_handle_ = nullptr;
}

Expand Down Expand Up @@ -610,8 +605,7 @@ class QueryWrap : public AsyncWrap {
static_cast<void*>(this));
}

static void CaresAsyncClose(uv_handle_t* handle) {
uv_async_t* async = reinterpret_cast<uv_async_t*>(handle);
static void CaresAsyncClose(uv_async_t* async) {
auto data = static_cast<struct CaresAsyncData*>(async->data);
delete data->wrap;
delete data;
Expand All @@ -636,7 +630,7 @@ class QueryWrap : public AsyncWrap {
free(host);
}

uv_close(reinterpret_cast<uv_handle_t*>(handle), CaresAsyncClose);
wrap->env()->CloseHandle(handle, CaresAsyncClose);
}

static void Callback(void *arg, int status, int timeouts,
Expand Down Expand Up @@ -1933,13 +1927,11 @@ void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
hints.ai_socktype = SOCK_STREAM;
hints.ai_flags = flags;

int err = uv_getaddrinfo(env->event_loop(),
req_wrap->req(),
AfterGetAddrInfo,
*hostname,
nullptr,
&hints);
req_wrap->Dispatched();
int err = req_wrap->Dispatch(uv_getaddrinfo,
AfterGetAddrInfo,
*hostname,
nullptr,
&hints);
if (err)
delete req_wrap;

Expand All @@ -1963,12 +1955,10 @@ void GetNameInfo(const FunctionCallbackInfo<Value>& args) {

GetNameInfoReqWrap* req_wrap = new GetNameInfoReqWrap(env, req_wrap_obj);

int err = uv_getnameinfo(env->event_loop(),
req_wrap->req(),
AfterGetNameInfo,
(struct sockaddr*)&addr,
NI_NAMEREQD);
req_wrap->Dispatched();
int err = req_wrap->Dispatch(uv_getnameinfo,
AfterGetNameInfo,
reinterpret_cast<struct sockaddr*>(&addr),
NI_NAMEREQD);
if (err)
delete req_wrap;

Expand Down
2 changes: 0 additions & 2 deletions src/connection_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ class ConnectionWrap : public LibuvStreamWrap {
ConnectionWrap(Environment* env,
v8::Local<v8::Object> object,
ProviderType provider);
~ConnectionWrap() {
}

UVType handle_;
};
Expand Down
62 changes: 60 additions & 2 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why not use a [&] lambda instead of the explicit closure struct?
Anyway, a line or two of comments would be nice:

// 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 T* handle and the uv_handle_t* handle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why not use a [&] lambda instead of the explicit closure struct?

Because that doesn’t work, it’s not convertible to the C signature that libuv needs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 [&] lambda doesn't work

Copy link
Member Author

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -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_;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is possible for someone to bump into a pending exception while this returns true?

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 may_call_into_js()? What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax In my understanding may_call_into_js() is probably not too different from can_call_into_js(), maybe add a comment there making it clear that this returning true does not guarantee that one can call into the JS without being careful about pending exceptions? Or, since we only set it to false...maybe cannot_call_into_js()?

Copy link
Member

Choose a reason for hiding this comment

The 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 is_in_tear_down()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment there making it clear that this returning true does not guarantee that one can call into the JS without being careful about pending exceptions?

Sure, I’m adding a comment. :)

Or, if we don't want to be too board about the meaning of this...maybe just something like is_in_tear_down()?

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();
}
Expand Down Expand Up @@ -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) \
Expand Down
Loading