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.

Sign up for GitHub

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

Jump to bottom

src: use std::optional for Worker thread id #41453

Merged
merged 2 commits into from
Jan 14, 2022
Merged

src: use std::optional for Worker thread id #41453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f177794
src: use std::optional for Worker thread id
addaleax Jan 9, 2022
8eb8c70
fixup! src: use std::optional for Worker thread id
addaleax Jan 13, 2022
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
20 changes: 11 additions & 9 deletions src/node_worker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,10 @@ bool Worker::CreateEnvMessagePort(Environment* env) {
}

void Worker::JoinThread() {
if (thread_joined_)
if (!tid_.has_value())
return;
CHECK_EQ(uv_thread_join(&tid_), 0);
thread_joined_ = true;
CHECK_EQ(uv_thread_join(&tid_.value()), 0);
tid_.reset();

env()->remove_sub_worker_context(this);

Expand All @@ -406,7 +406,7 @@ void Worker::JoinThread() {
MakeCallback(env()->onexit_string(), arraysize(args), args);
}

// If we get here, the !thread_joined_ condition at the top of the function
// If we get here, the tid_.has_value() condition at the top of the function
// implies that the thread was running. In that case, its final action will
// be to schedule a callback on the parent thread which will delete this
// object, so there's nothing more to do here.
Expand All @@ -417,7 +417,7 @@ Worker::~Worker() {

CHECK(stopped_);
CHECK_NULL(env_);
CHECK(thread_joined_);
CHECK(!tid_.has_value());

Debug(this, "Worker %llu destroyed", thread_id_.id);
}
Expand Down Expand Up @@ -600,7 +600,9 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
uv_thread_options_t thread_options;
thread_options.flags = UV_THREAD_HAS_STACK_SIZE;
thread_options.stack_size = w->stack_size_;
int ret = uv_thread_create_ex(&w->tid_, &thread_options, [](void* arg) {

uv_thread_t* tid = &w->tid_.emplace(); // Create uv_thread_t instance
int ret = uv_thread_create_ex(tid, &thread_options, [](void* arg) {
// XXX: This could become a std::unique_ptr, but that makes at least
// gcc 6.3 detect undefined behaviour when there shouldn't be any.
// gcc 7+ handles this well.
Expand All @@ -627,14 +629,14 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
// The object now owns the created thread and should not be garbage
// collected until that finishes.
w->ClearWeak();
w->thread_joined_ = false;

if (w->has_ref_)
w->env()->add_refs(1);

w->env()->add_sub_worker_context(w);
} else {
w->stopped_ = true;
w->tid_.reset();

char err_buf[128];
uv_err_name_r(ret, err_buf, sizeof(err_buf));
Expand All @@ -657,7 +659,7 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
void Worker::Ref(const FunctionCallbackInfo<Value>& args) {
Worker* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.This());
if (!w->has_ref_ && !w->thread_joined_) {
if (!w->has_ref_ && w->tid_.has_value()) {
w->has_ref_ = true;
w->env()->add_refs(1);
}
Expand All @@ -666,7 +668,7 @@ void Worker::Ref(const FunctionCallbackInfo<Value>& args) {
void Worker::Unref(const FunctionCallbackInfo<Value>& args) {
Worker* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.This());
if (w->has_ref_ && !w->thread_joined_) {
if (w->has_ref_ && w->tid_.has_value()) {
w->has_ref_ = false;
w->env()->add_refs(-1);
}
Expand Down
3 changes: 1 addition & 2 deletions src/node_worker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,13 @@ class Worker : public AsyncWrap {

MultiIsolatePlatform* platform_;
v8::Isolate* isolate_ = nullptr;
uv_thread_t tid_;
std::optional<uv_thread_t> tid_; // Set while the thread is running

std::unique_ptr<InspectorParentHandle> inspector_parent_handle_;

// This mutex protects access to all variables listed below it.
mutable Mutex mutex_;

bool thread_joined_ = 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.

The comment here implied that this was protected by the mutex, but a) that didn’t reflect the actual usage of this field and b) it’s only ever accessed from the parent thread anyway, so no locking is necessary here

const char* custom_error_ = nullptr;
std::string custom_error_str_;
int exit_code_ = 0;
Expand Down