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: use enum for refed flag on native immediates #33444

Closed
wants to merge 1 commit 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
2 changes: 1 addition & 1 deletion src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
}

if (env->destroy_async_id_list()->empty()) {
env->SetUnrefImmediate(&DestroyAsyncIdsCallback);
env->SetImmediate(&DestroyAsyncIdsCallback, CallbackFlags::kUnrefed);
}

env->destroy_async_id_list()->push_back(async_id);
Expand Down
16 changes: 8 additions & 8 deletions src/callback_queue-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ namespace node {
template <typename R, typename... Args>
template <typename Fn>
std::unique_ptr<typename CallbackQueue<R, Args...>::Callback>
CallbackQueue<R, Args...>::CreateCallback(Fn&& fn, bool refed) {
return std::make_unique<CallbackImpl<Fn>>(std::move(fn), refed);
CallbackQueue<R, Args...>::CreateCallback(Fn&& fn, CallbackFlags::Flags flags) {
return std::make_unique<CallbackImpl<Fn>>(std::move(fn), flags);
}

template <typename R, typename... Args>
Expand Down Expand Up @@ -57,12 +57,12 @@ size_t CallbackQueue<R, Args...>::size() const {
}

template <typename R, typename... Args>
CallbackQueue<R, Args...>::Callback::Callback(bool refed)
: refed_(refed) {}
CallbackQueue<R, Args...>::Callback::Callback(CallbackFlags::Flags flags)
: flags_(flags) {}

template <typename R, typename... Args>
bool CallbackQueue<R, Args...>::Callback::is_refed() const {
return refed_;
CallbackFlags::Flags CallbackQueue<R, Args...>::Callback::flags() const {
return flags_;
}

template <typename R, typename... Args>
Expand All @@ -80,8 +80,8 @@ void CallbackQueue<R, Args...>::Callback::set_next(
template <typename R, typename... Args>
template <typename Fn>
CallbackQueue<R, Args...>::CallbackImpl<Fn>::CallbackImpl(
Fn&& callback, bool refed)
: Callback(refed),
Fn&& callback, CallbackFlags::Flags flags)
: Callback(flags),
callback_(std::move(callback)) {}

template <typename R, typename... Args>
Expand Down
18 changes: 13 additions & 5 deletions src/callback_queue.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@

namespace node {

namespace CallbackFlags {
enum Flags {
kUnrefed = 0,
kRefed = 1,
};
}

// A queue of C++ functions that take Args... as arguments and return R
// (this is similar to the signature of std::function).
// New entries are added using `CreateCallback()`/`Push()`, and removed using
Expand All @@ -18,25 +25,26 @@ class CallbackQueue {
public:
class Callback {
public:
explicit inline Callback(bool refed);
explicit inline Callback(CallbackFlags::Flags flags);

virtual ~Callback() = default;
virtual R Call(Args... args) = 0;

inline bool is_refed() const;
inline CallbackFlags::Flags flags() const;

private:
inline std::unique_ptr<Callback> get_next();
inline void set_next(std::unique_ptr<Callback> next);

bool refed_;
CallbackFlags::Flags flags_;
std::unique_ptr<Callback> next_;

friend class CallbackQueue;
};

template <typename Fn>
inline std::unique_ptr<Callback> CreateCallback(Fn&& fn, bool refed);
inline std::unique_ptr<Callback> CreateCallback(
Fn&& fn, CallbackFlags::Flags);

inline std::unique_ptr<Callback> Shift();
inline void Push(std::unique_ptr<Callback> cb);
Expand All @@ -51,7 +59,7 @@ class CallbackQueue {
template <typename Fn>
class CallbackImpl final : public Callback {
public:
CallbackImpl(Fn&& callback, bool refed);
CallbackImpl(Fn&& callback, CallbackFlags::Flags flags);
R Call(Args... args) override;

private:
Expand Down
32 changes: 12 additions & 20 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -707,29 +707,21 @@ inline void IsolateData::set_options(
}

template <typename Fn>
void Environment::CreateImmediate(Fn&& cb, bool ref) {
auto callback = native_immediates_.CreateCallback(std::move(cb), ref);
void Environment::SetImmediate(Fn&& cb, CallbackFlags::Flags flags) {
auto callback = native_immediates_.CreateCallback(std::move(cb), flags);
native_immediates_.Push(std::move(callback));
}

template <typename Fn>
void Environment::SetImmediate(Fn&& cb) {
CreateImmediate(std::move(cb), true);

if (immediate_info()->ref_count() == 0)
ToggleImmediateRef(true);
immediate_info()->ref_count_inc(1);
}

template <typename Fn>
void Environment::SetUnrefImmediate(Fn&& cb) {
CreateImmediate(std::move(cb), false);
if (flags & CallbackFlags::kRefed) {
if (immediate_info()->ref_count() == 0)
ToggleImmediateRef(true);
immediate_info()->ref_count_inc(1);
}
}

template <typename Fn>
void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) {
auto callback =
native_immediates_threadsafe_.CreateCallback(std::move(cb), refed);
void Environment::SetImmediateThreadsafe(Fn&& cb, CallbackFlags::Flags flags) {
auto callback = native_immediates_threadsafe_.CreateCallback(
std::move(cb), flags);
{
Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
native_immediates_threadsafe_.Push(std::move(callback));
Expand All @@ -740,8 +732,8 @@ void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) {

template <typename Fn>
void Environment::RequestInterrupt(Fn&& cb) {
auto callback =
native_immediates_interrupts_.CreateCallback(std::move(cb), false);
auto callback = native_immediates_interrupts_.CreateCallback(
std::move(cb), CallbackFlags::kRefed);
{
Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
native_immediates_interrupts_.Push(std::move(callback));
Expand Down
7 changes: 4 additions & 3 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ void Environment::CleanupHandles() {
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(),
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);

RunAndClearNativeImmediates(true /* skip SetUnrefImmediate()s */);
RunAndClearNativeImmediates(true /* skip unrefed SetImmediate()s */);

for (ReqWrapBase* request : req_wrap_queue_)
request->Cancel();
Expand Down Expand Up @@ -745,10 +745,11 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
TryCatchScope try_catch(this);
DebugSealHandleScope seal_handle_scope(isolate());
while (auto head = queue->Shift()) {
if (head->is_refed())
bool is_refed = head->flags() & CallbackFlags::kRefed;
if (is_refed)
ref_count++;

if (head->is_refed() || !only_refed)
if (is_refed || !only_refed)
head->Call(this);

head.reset(); // Destroy now so that this is also observed by try_catch.
Expand Down
11 changes: 4 additions & 7 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1164,12 +1164,12 @@ class Environment : public MemoryRetainer {
// Unlike the JS setImmediate() function, nested SetImmediate() calls will
// be run without returning control to the event loop, similar to nextTick().
template <typename Fn>
inline void SetImmediate(Fn&& cb);
template <typename Fn>
inline void SetUnrefImmediate(Fn&& cb);
inline void SetImmediate(
Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed);
template <typename Fn>
// This behaves like SetImmediate() but can be called from any thread.
inline void SetImmediateThreadsafe(Fn&& cb, bool refed = true);
inline void SetImmediateThreadsafe(
Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed);
// This behaves like V8's Isolate::RequestInterrupt(), but also accounts for
// the event loop (i.e. combines the V8 function with SetImmediate()).
// The passed callback may not throw exceptions.
Expand Down Expand Up @@ -1252,9 +1252,6 @@ class Environment : public MemoryRetainer {
void RunAndClearInterrupts();

private:
template <typename Fn>
inline void CreateImmediate(Fn&& cb, bool ref);

inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);

Expand Down
4 changes: 2 additions & 2 deletions src/node_dir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,10 @@ inline void DirHandle::GCClose() {
// to notify that the file descriptor was gc'd. We want to be noisy about
// this because not explicitly closing the DirHandle is a bug.

env()->SetUnrefImmediate([](Environment* env) {
env()->SetImmediate([](Environment* env) {
ProcessEmitWarning(env,
"Closing directory handle on garbage collection");
});
}, CallbackFlags::kUnrefed);
}

void AfterClose(uv_fs_t* req) {
Expand Down
4 changes: 2 additions & 2 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ inline void FileHandle::Close() {
// to notify that the file descriptor was gc'd. We want to be noisy about
// this because not explicitly closing the FileHandle is a bug.

env()->SetUnrefImmediate([detail](Environment* env) {
env()->SetImmediate([detail](Environment* env) {
ProcessEmitWarning(env,
"Closing file descriptor %d on garbage collection",
detail.fd);
Expand All @@ -240,7 +240,7 @@ inline void FileHandle::Close() {
"thrown if a file descriptor is closed during garbage collection.",
"DEP0137").IsNothing();
}
});
}, CallbackFlags::kUnrefed);
}

void FileHandle::CloseReq::Resolve() {
Expand Down
4 changes: 2 additions & 2 deletions src/node_perf.cc
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,9 @@ void MarkGarbageCollectionEnd(Isolate* isolate,
static_cast<PerformanceGCFlags>(flags),
state->performance_last_gc_start_mark,
PERFORMANCE_NOW());
env->SetUnrefImmediate([entry = std::move(entry)](Environment* env) mutable {
env->SetImmediate([entry = std::move(entry)](Environment* env) mutable {
PerformanceGCCallback(env, std::move(entry));
});
}, CallbackFlags::kUnrefed);
}

void GarbageCollectionCleanupHook(void* data) {
Expand Down
2 changes: 1 addition & 1 deletion src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
env, std::move(snapshot));
Local<Value> args[] = { stream->object() };
taker->MakeCallback(env->ondone_string(), arraysize(args), args);
}, /* refed */ false);
}, CallbackFlags::kUnrefed);
});
args.GetReturnValue().Set(scheduled ? taker->object() : Local<Object>());
}
Expand Down
6 changes: 3 additions & 3 deletions test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,11 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) {
(*env)->SetImmediate([&](node::Environment* env_arg) {
EXPECT_EQ(env_arg, *env);
called++;
});
(*env)->SetUnrefImmediate([&](node::Environment* env_arg) {
}, node::CallbackFlags::kRefed);
(*env)->SetImmediate([&](node::Environment* env_arg) {
EXPECT_EQ(env_arg, *env);
called_unref++;
});
}, node::CallbackFlags::kUnrefed);
}

EXPECT_EQ(called, 1);
Expand Down