Skip to content

src: set thread local env in CreateEnvironment #18573

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

Closed
wants to merge 5 commits into from
Closed

src: set thread local env in CreateEnvironment #18573

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2ae326
src: set thread local env in CreateEnvironment
danbev Feb 5, 2018
e047cb0
squash: remove this for Env contstructor call
danbev Feb 6, 2018
6dee9c8
squash: move code into Environment to avoid dupl
danbev Feb 13, 2018
3bceaee
squash: use uv_once_t to create the thread local
danbev Feb 15, 2018
b5761a7
squash: remove white spaces added
danbev Feb 16, 2018
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
9 changes: 9 additions & 0 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,10 @@ inline Environment* Environment::GetCurrent(
info.Data().template As<v8::External>()->Value());
}

inline Environment* Environment::GetThreadLocalEnv() {
return static_cast<Environment*>(uv_key_get(&thread_local_env));
}

inline Environment::Environment(IsolateData* isolate_data,
v8::Local<v8::Context> context)
: isolate_(context->GetIsolate()),
Expand Down Expand Up @@ -369,6 +373,11 @@ inline Environment::~Environment() {
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
#undef V

auto tl_env = static_cast<Environment*>(uv_key_get(&thread_local_env));
if (tl_env == this) {
uv_key_delete(&Environment::thread_local_env);
}

delete[] heap_statistics_buffer_;
delete[] heap_space_statistics_buffer_;
delete[] http_parser_buffer_;
Expand Down
5 changes: 5 additions & 0 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ void Environment::Start(int argc,

SetupProcessObject(this, argc, argv, exec_argc, exec_argv);
LoadAsyncWrapperInfo(this);

CHECK_EQ(0, uv_key_create(&thread_local_env));
Copy link
Member

Choose a reason for hiding this comment

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

uv_key_create() and uv_key_delete() should be called only once, not for every environment. (The current initialization logic is probably wrong too.)

What should work is using a uv_once_t to protect the call to uv_key_create() and simply don't bother with uv_key_delete().

uv_key_set(&thread_local_env, this);
}

void Environment::CleanupHandles() {
Expand Down Expand Up @@ -471,4 +474,6 @@ void Environment::AsyncHooks::grow_async_ids_stack() {
async_ids_stack_.GetJSArray()).FromJust();
}

uv_key_t Environment::thread_local_env = {};

} // namespace node
5 changes: 5 additions & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ struct ContextInfo {
bool is_default = false;
};


Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the whitespace change here and on line 839?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed them and will remove them now. Thanks

class Environment {
public:
class AsyncHooks {
Expand Down Expand Up @@ -544,6 +545,8 @@ class Environment {
static inline Environment* GetCurrent(
const v8::PropertyCallbackInfo<T>& info);

static inline Environment* GetThreadLocalEnv();

inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
inline ~Environment();

Expand Down Expand Up @@ -832,6 +835,8 @@ class Environment {
v8::Local<v8::Promise> promise,
v8::Local<v8::Value> parent);

static uv_key_t thread_local_env;

#define V(PropertyName, TypeName) \
v8::Persistent<TypeName> PropertyName ## _;
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
Expand Down
8 changes: 1 addition & 7 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4207,11 +4207,8 @@ uv_loop_t* GetCurrentEventLoop(v8::Isolate* isolate) {
}


static uv_key_t thread_local_env;


void AtExit(void (*cb)(void* arg), void* arg) {
auto env = static_cast<Environment*>(uv_key_get(&thread_local_env));
auto env = Environment::GetThreadLocalEnv();
AtExit(env, cb, arg);
}

Expand Down Expand Up @@ -4342,8 +4339,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
Local<Context> context = NewContext(isolate);
Context::Scope context_scope(context);
Environment env(isolate_data, context);
CHECK_EQ(0, uv_key_create(&thread_local_env));
uv_key_set(&thread_local_env, &env);
env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);

const char* path = argc > 1 ? argv[1] : nullptr;
Expand Down Expand Up @@ -4393,7 +4388,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,

const int exit_code = EmitExit(&env);
RunAtExit(&env);
uv_key_delete(&thread_local_env);

v8_platform.DrainVMTasks(isolate);
v8_platform.CancelVMTasks(isolate);
Expand Down
10 changes: 10 additions & 0 deletions test/cctest/test_environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ TEST_F(EnvironmentTest, AtExitWithEnvironment) {
EXPECT_TRUE(called_cb_1);
}

TEST_F(EnvironmentTest, AtExitWithoutEnvironment) {
const v8::HandleScope handle_scope(isolate_);
const Argv argv;
Env env {handle_scope, argv};

AtExit(at_exit_callback1); // No Environment is passed to AtExit.
RunAtExit(*env);
EXPECT_TRUE(called_cb_1);
}

TEST_F(EnvironmentTest, AtExitWithArgument) {
const v8::HandleScope handle_scope(isolate_);
const Argv argv;
Expand Down