Skip to content

Commit 5207dec

Browse files
addaleaxaddaleax
committed
src: allow generic C++ callables in SetImmediate()
Modify the native `SetImmediate()` functions to take generic C++ callables as arguments. This makes passing arguments to the callback easier, and in particular, it allows passing `std::unique_ptr`s directly, which in turn makes sure that the data they point to is deleted if the `Environment` is torn down before the callback can run. PR-URL: nodejs#28704 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 61f3a5c commit 5207dec

File tree

12 files changed

+206
-163
lines changed

12 files changed

+206
-163
lines changed

‎src/async_wrap.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ struct AsyncWrapObject : public AsyncWrap {
8787
SET_SELF_SIZE(AsyncWrapObject)
8888
};
8989

90-
void AsyncWrap::DestroyAsyncIdsCallback(Environment* env, void* data) {
90+
void AsyncWrap::DestroyAsyncIdsCallback(Environment* env) {
9191
Local<Function> fn = env->async_hooks_destroy_function();
9292

9393
TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal);
@@ -642,7 +642,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
642642
}
643643

644644
if (env->destroy_async_id_list()->empty()) {
645-
env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr);
645+
env->SetUnrefImmediate(&DestroyAsyncIdsCallback);
646646
}
647647

648648
env->destroy_async_id_list()->push_back(async_id);

‎src/async_wrap.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ class AsyncWrap : public BaseObject {
154154
static void EmitTraceEventAfter(ProviderType type, double async_id);
155155
void EmitTraceEventDestroy();
156156

157-
static void DestroyAsyncIdsCallback(Environment* env, void* data);
157+
static void DestroyAsyncIdsCallback(Environment* env);
158158

159159
inline ProviderType provider_type() const;
160160
inline ProviderType set_provider_type(ProviderType provider);

‎src/cares_wrap.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -690,9 +690,9 @@ class QueryWrap : public AsyncWrap {
690690
}
691691

692692
void QueueResponseCallback(int status) {
693-
env()->SetImmediate([](Environment*, void* data) {
694-
static_cast<QueryWrap*>(data)->AfterResponse();
695-
}, this, object());
693+
env()->SetImmediate([this](Environment*) {
694+
AfterResponse();
695+
}, object());
696696

697697
channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
698698
channel_->ModifyActivityQueryCount(-1);

‎src/env-inl.h‎

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -753,33 +753,66 @@ inline void IsolateData::set_options(
753753
options_ = std::move(options);
754754
}
755755

756-
void Environment::CreateImmediate(native_immediate_callback cb,
757-
void* data,
758-
v8::Local<v8::Object> obj,
756+
template <typename Fn>
757+
void Environment::CreateImmediate(Fn&& cb,
758+
v8::Local<v8::Object> keep_alive,
759759
bool ref) {
760-
native_immediate_callbacks_.push_back({
761-
cb,
762-
data,
763-
v8::Global<v8::Object>(isolate_, obj),
764-
ref
765-
});
760+
auto callback = std::make_unique<NativeImmediateCallbackImpl<Fn>>(
761+
std::move(cb),
762+
v8::Global<v8::Object>(isolate(), keep_alive),
763+
ref);
764+
NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_;
765+
766+
native_immediate_callbacks_tail_ = callback.get();
767+
if (prev_tail != nullptr)
768+
prev_tail->set_next(std::move(callback));
769+
else
770+
native_immediate_callbacks_head_ = std::move(callback);
771+
766772
immediate_info()->count_inc(1);
767773
}
768774

769-
void Environment::SetImmediate(native_immediate_callback cb,
770-
void* data,
771-
v8::Local<v8::Object> obj) {
772-
CreateImmediate(cb, data, obj, true);
775+
template <typename Fn>
776+
void Environment::SetImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
777+
CreateImmediate(std::move(cb), keep_alive, true);
773778

774779
if (immediate_info()->ref_count() == 0)
775780
ToggleImmediateRef(true);
776781
immediate_info()->ref_count_inc(1);
777782
}
778783

779-
void Environment::SetUnrefImmediate(native_immediate_callback cb,
780-
void* data,
781-
v8::Local<v8::Object> obj) {
782-
CreateImmediate(cb, data, obj, false);
784+
template <typename Fn>
785+
void Environment::SetUnrefImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
786+
CreateImmediate(std::move(cb), keep_alive, false);
787+
}
788+
789+
Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed)
790+
: refed_(refed) {}
791+
792+
bool Environment::NativeImmediateCallback::is_refed() const {
793+
return refed_;
794+
}
795+
796+
std::unique_ptr<Environment::NativeImmediateCallback>
797+
Environment::NativeImmediateCallback::get_next() {
798+
return std::move(next_);
799+
}
800+
801+
void Environment::NativeImmediateCallback::set_next(
802+
std::unique_ptr<NativeImmediateCallback> next) {
803+
next_ = std::move(next);
804+
}
805+
806+
template <typename Fn>
807+
Environment::NativeImmediateCallbackImpl<Fn>::NativeImmediateCallbackImpl(
808+
Fn&& callback, v8::Global<v8::Object>&& keep_alive, bool refed)
809+
: NativeImmediateCallback(refed),
810+
callback_(std::move(callback)),
811+
keep_alive_(std::move(keep_alive)) {}
812+
813+
template <typename Fn>
814+
void Environment::NativeImmediateCallbackImpl<Fn>::Call(Environment* env) {
815+
callback_(env);
783816
}
784817

785818
inline bool Environment::can_call_into_js() const {

‎src/env.cc‎

Lines changed: 31 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ Environment::Environment(IsolateData* isolate_data,
339339
[](void* arg) {
340340
Environment* env = static_cast<Environment*>(arg);
341341
if (!env->destroy_async_id_list()->empty())
342-
AsyncWrap::DestroyAsyncIdsCallback(env, nullptr);
342+
AsyncWrap::DestroyAsyncIdsCallback(env);
343343
},
344344
this);
345345

@@ -642,42 +642,38 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
642642
void Environment::RunAndClearNativeImmediates() {
643643
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
644644
"RunAndClearNativeImmediates", this);
645-
size_t count = native_immediate_callbacks_.size();
646-
if (count > 0) {
647-
size_t ref_count = 0;
648-
std::vector<NativeImmediateCallback> list;
649-
native_immediate_callbacks_.swap(list);
650-
auto drain_list = [&]() {
651-
TryCatchScope try_catch(this);
652-
for (auto it = list.begin(); it != list.end(); ++it) {
653-
DebugSealHandleScope seal_handle_scope(isolate());
654-
it->cb_(this, it->data_);
655-
if (it->refed_)
656-
ref_count++;
657-
if (UNLIKELY(try_catch.HasCaught())) {
658-
if (!try_catch.HasTerminated())
659-
errors::TriggerUncaughtException(isolate(), try_catch);
660-
661-
// We are done with the current callback. Increase the counter so that
662-
// the steps below make everything *after* the current item part of
663-
// the new list.
664-
it++;
665-
666-
// Bail out, remove the already executed callbacks from list
667-
// and set up a new TryCatch for the other pending callbacks.
668-
std::move_backward(it, list.end(), list.begin() + (list.end() - it));
669-
list.resize(list.end() - it);
670-
return true;
671-
}
645+
size_t ref_count = 0;
646+
size_t count = 0;
647+
std::unique_ptr<NativeImmediateCallback> head;
648+
head.swap(native_immediate_callbacks_head_);
649+
native_immediate_callbacks_tail_ = nullptr;
650+
651+
auto drain_list = [&]() {
652+
TryCatchScope try_catch(this);
653+
for (; head; head = head->get_next()) {
654+
DebugSealHandleScope seal_handle_scope(isolate());
655+
count++;
656+
if (head->is_refed())
657+
ref_count++;
658+
659+
head->Call(this);
660+
if (UNLIKELY(try_catch.HasCaught())) {
661+
if (!try_catch.HasTerminated())
662+
errors::TriggerUncaughtException(isolate(), try_catch);
663+
664+
// We are done with the current callback. Move one iteration along,
665+
// as if we had completed successfully.
666+
head = head->get_next();
667+
return true;
672668
}
673-
return false;
674-
};
675-
while (drain_list()) {}
669+
}
670+
return false;
671+
};
672+
while (head && drain_list()) {}
676673

677-
DCHECK_GE(immediate_info()->count(), count);
678-
immediate_info()->count_dec(count);
679-
immediate_info()->ref_count_dec(ref_count);
680-
}
674+
DCHECK_GE(immediate_info()->count(), count);
675+
immediate_info()->count_dec(count);
676+
immediate_info()->ref_count_dec(ref_count);
681677
}
682678

683679

‎src/env.h‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,15 +1175,15 @@ class Environment : public MemoryRetainer {
11751175
return current_value;
11761176
}
11771177

1178-
typedef void (*native_immediate_callback)(Environment* env, void* data);
1179-
// cb will be called as cb(env, data) on the next event loop iteration.
1180-
// obj will be kept alive between now and after the callback has run.
1181-
inline void SetImmediate(native_immediate_callback cb,
1182-
void* data,
1183-
v8::Local<v8::Object> obj = v8::Local<v8::Object>());
1184-
inline void SetUnrefImmediate(native_immediate_callback cb,
1185-
void* data,
1186-
v8::Local<v8::Object> obj =
1178+
// cb will be called as cb(env) on the next event loop iteration.
1179+
// keep_alive will be kept alive between now and after the callback has run.
1180+
template <typename Fn>
1181+
inline void SetImmediate(Fn&& cb,
1182+
v8::Local<v8::Object> keep_alive =
1183+
v8::Local<v8::Object>());
1184+
template <typename Fn>
1185+
inline void SetUnrefImmediate(Fn&& cb,
1186+
v8::Local<v8::Object> keep_alive =
11871187
v8::Local<v8::Object>());
11881188
// This needs to be available for the JS-land setImmediate().
11891189
void ToggleImmediateRef(bool ref);
@@ -1248,9 +1248,9 @@ class Environment : public MemoryRetainer {
12481248
#endif // HAVE_INSPECTOR
12491249

12501250
private:
1251-
inline void CreateImmediate(native_immediate_callback cb,
1252-
void* data,
1253-
v8::Local<v8::Object> obj,
1251+
template <typename Fn>
1252+
inline void CreateImmediate(Fn&& cb,
1253+
v8::Local<v8::Object> keep_alive,
12541254
bool ref);
12551255

12561256
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
@@ -1374,13 +1374,38 @@ class Environment : public MemoryRetainer {
13741374

13751375
std::list<ExitCallback> at_exit_functions_;
13761376

1377-
struct NativeImmediateCallback {
1378-
native_immediate_callback cb_;
1379-
void* data_;
1380-
v8::Global<v8::Object> keep_alive_;
1377+
class NativeImmediateCallback {
1378+
public:
1379+
explicit inline NativeImmediateCallback(bool refed);
1380+
1381+
virtual ~NativeImmediateCallback() = default;
1382+
virtual void Call(Environment* env) = 0;
1383+
1384+
inline bool is_refed() const;
1385+
inline std::unique_ptr<NativeImmediateCallback> get_next();
1386+
inline void set_next(std::unique_ptr<NativeImmediateCallback> next);
1387+
1388+
private:
13811389
bool refed_;
1390+
std::unique_ptr<NativeImmediateCallback> next_;
1391+
};
1392+
1393+
template <typename Fn>
1394+
class NativeImmediateCallbackImpl final : public NativeImmediateCallback {
1395+
public:
1396+
NativeImmediateCallbackImpl(Fn&& callback,
1397+
v8::Global<v8::Object>&& keep_alive,
1398+
bool refed);
1399+
void Call(Environment* env) override;
1400+
1401+
private:
1402+
Fn callback_;
1403+
v8::Global<v8::Object> keep_alive_;
13821404
};
1383-
std::vector<NativeImmediateCallback> native_immediate_callbacks_;
1405+
1406+
std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;
1407+
NativeImmediateCallback* native_immediate_callbacks_tail_ = nullptr;
1408+
13841409
void RunAndClearNativeImmediates();
13851410
static void CheckImmediate(uv_check_t* handle);
13861411

‎src/node_api.cc‎

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -36,27 +36,33 @@ class BufferFinalizer : private Finalizer {
3636
public:
3737
// node::Buffer::FreeCallback
3838
static void FinalizeBufferCallback(char* data, void* hint) {
39-
BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);
39+
std::unique_ptr<BufferFinalizer, Deleter> finalizer{
40+
static_cast<BufferFinalizer*>(hint)};
4041
finalizer->_finalize_data = data;
41-
static_cast<node_napi_env>(finalizer->_env)->node_env()
42-
->SetImmediate([](node::Environment* env, void* hint) {
43-
BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);
44-
45-
if (finalizer->_finalize_callback != nullptr) {
46-
v8::HandleScope handle_scope(finalizer->_env->isolate);
47-
v8::Context::Scope context_scope(finalizer->_env->context());
48-
49-
finalizer->_env->CallIntoModuleThrow([&](napi_env env) {
50-
finalizer->_finalize_callback(
51-
env,
52-
finalizer->_finalize_data,
53-
finalizer->_finalize_hint);
54-
});
55-
}
5642

57-
Delete(finalizer);
58-
}, hint);
43+
node::Environment* node_env =
44+
static_cast<node_napi_env>(finalizer->_env)->node_env();
45+
node_env->SetImmediate(
46+
[finalizer = std::move(finalizer)](node::Environment* env) {
47+
if (finalizer->_finalize_callback == nullptr) return;
48+
49+
v8::HandleScope handle_scope(finalizer->_env->isolate);
50+
v8::Context::Scope context_scope(finalizer->_env->context());
51+
52+
finalizer->_env->CallIntoModuleThrow([&](napi_env env) {
53+
finalizer->_finalize_callback(
54+
env,
55+
finalizer->_finalize_data,
56+
finalizer->_finalize_hint);
57+
});
58+
});
5959
}
60+
61+
struct Deleter {
62+
void operator()(BufferFinalizer* finalizer) {
63+
Finalizer::Delete(finalizer);
64+
}
65+
};
6066
};
6167

6268
static inline napi_env NewEnv(v8::Local<v8::Context> context) {

‎src/node_file.cc‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -170,35 +170,33 @@ inline void FileHandle::Close() {
170170

171171
struct err_detail { int ret; int fd; };
172172

173-
err_detail* detail = new err_detail { ret, fd_ };
173+
err_detail detail { ret, fd_ };
174174

175175
if (ret < 0) {
176176
// Do not unref this
177-
env()->SetImmediate([](Environment* env, void* data) {
177+
env()->SetImmediate([detail](Environment* env) {
178178
char msg[70];
179-
std::unique_ptr<err_detail> detail(static_cast<err_detail*>(data));
180179
snprintf(msg, arraysize(msg),
181180
"Closing file descriptor %d on garbage collection failed",
182-
detail->fd);
181+
detail.fd);
183182
// This exception will end up being fatal for the process because
184183
// it is being thrown from within the SetImmediate handler and
185184
// there is no JS stack to bubble it to. In other words, tearing
186185
// down the process is the only reasonable thing we can do here.
187186
HandleScope handle_scope(env->isolate());
188-
env->ThrowUVException(detail->ret, "close", msg);
189-
}, detail);
187+
env->ThrowUVException(detail.ret, "close", msg);
188+
});
190189
return;
191190
}
192191

193192
// If the close was successful, we still want to emit a process warning
194193
// to notify that the file descriptor was gc'd. We want to be noisy about
195194
// this because not explicitly closing the FileHandle is a bug.
196-
env()->SetUnrefImmediate([](Environment* env, void* data) {
197-
std::unique_ptr<err_detail> detail(static_cast<err_detail*>(data));
195+
env()->SetUnrefImmediate([detail](Environment* env) {
198196
ProcessEmitWarning(env,
199197
"Closing file descriptor %d on garbage collection",
200-
detail->fd);
201-
}, detail);
198+
detail.fd);
199+
});
202200
}
203201

204202
void FileHandle::CloseReq::Resolve() {

0 commit comments

Comments
 (0)