Skip to content

Commit

Permalink
src: distinguish env stopping flags
Browse files Browse the repository at this point in the history
`Environment::FreeEnvironment` creates a
`DisallowJavascriptExecutionScope`, so the flag
`Environment::can_call_into_js()` should also be set as `false`. As
`Environment::can_call_into_js_` is a simple boolean flag compared to
the atomic `Environment::is_stopping_`, call sites that are
determined to be on the same thread of the JavaScript execution
should prefer `Environment::can_call_into_js_`.
  • Loading branch information
legendecas committed Dec 19, 2022
1 parent 1d6fda0 commit ac5ee86
Show file tree
Hide file tree
Showing 8 changed files with 18 additions and 12 deletions.
6 changes: 4 additions & 2 deletions src/api/environment.cc
Expand Up @@ -52,8 +52,7 @@ bool AllowWasmCodeGenerationCallback(Local<Context> context,
bool ShouldAbortOnUncaughtException(Isolate* isolate) {
DebugSealHandleScope scope(isolate);
Environment* env = Environment::GetCurrent(isolate);
return env != nullptr &&
(env->is_main_thread() || !env->is_stopping()) &&
return env != nullptr && (env->is_main_thread() || env->can_call_into_js()) &&
env->abort_on_uncaught_exception() &&
env->should_abort_on_uncaught_toggle()[0] &&
!env->inside_should_not_abort_on_uncaught_scope();
Expand Down Expand Up @@ -404,6 +403,9 @@ void FreeEnvironment(Environment* env) {
Context::Scope context_scope(env->context());
SealHandleScope seal_handle_scope(isolate);

// Set the flag in accordance with the DisallowJavascriptExecutionScope
// above.
env->set_can_call_into_js(false);
env->set_stopping(true);
env->stop_sub_worker_contexts();
env->RunCleanup();
Expand Down
2 changes: 1 addition & 1 deletion src/env-inl.h
Expand Up @@ -613,7 +613,7 @@ void Environment::RequestInterrupt(Fn&& cb) {
}

inline bool Environment::can_call_into_js() const {
return can_call_into_js_ && !is_stopping();
return can_call_into_js_;
}

inline void Environment::set_can_call_into_js(bool can_call_into_js) {
Expand Down
9 changes: 6 additions & 3 deletions src/env.cc
Expand Up @@ -809,7 +809,7 @@ Environment::~Environment() {
}

// FreeEnvironment() should have set this.
CHECK(is_stopping());
CHECK(!can_call_into_js());

if (heapsnapshot_near_heap_limit_callback_added_) {
RemoveHeapSnapshotNearHeapLimitCallback(0);
Expand Down Expand Up @@ -905,10 +905,13 @@ void Environment::InitializeLibuv() {
}

void Environment::ExitEnv() {
set_can_call_into_js(false);
// Should not access non-thread-safe methods here.
set_stopping(true);
isolate_->TerminateExecution();
SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); });
SetImmediateThreadsafe([](Environment* env) {
env->set_can_call_into_js(false);
uv_stop(env->event_loop());
});
}

void Environment::RegisterHandleCleanups() {
Expand Down
3 changes: 3 additions & 0 deletions src/env.h
Expand Up @@ -781,6 +781,9 @@ class Environment : public MemoryRetainer {
void stop_sub_worker_contexts();
template <typename Fn>
inline void ForEachWorker(Fn&& iterator);
// Determine if the environment is stopping. This getter is thread-safe.
// If the check is not performed off thread, the flag
// `env->can_call_into_js()` should be preferred.
inline bool is_stopping() const;
inline void set_stopping(bool value);
inline std::list<node_module>* extra_linked_bindings();
Expand Down
3 changes: 1 addition & 2 deletions src/module_wrap.cc
Expand Up @@ -410,8 +410,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {

// Convert the termination exception into a regular exception.
if (timed_out || received_signal) {
if (!env->is_main_thread() && env->is_stopping())
return;
if (!env->is_main_thread() && !env->can_call_into_js()) return;
env->isolate()->CancelTerminateExecution();
// It is possible that execution was terminated by another timeout in
// which this timeout is nested, so check whether one of the watchdogs
Expand Down
3 changes: 1 addition & 2 deletions src/node_contextify.cc
Expand Up @@ -1028,8 +1028,7 @@ bool ContextifyScript::EvalMachine(Local<Context> context,

// Convert the termination exception into a regular exception.
if (timed_out || received_signal) {
if (!env->is_main_thread() && env->is_stopping())
return false;
if (!env->is_main_thread() && !env->can_call_into_js()) return false;
env->isolate()->CancelTerminateExecution();
// It is possible that execution was terminated by another timeout in
// which this timeout is nested, so check whether one of the watchdogs
Expand Down
2 changes: 1 addition & 1 deletion src/node_http2.cc
Expand Up @@ -1127,7 +1127,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
// Don't close synchronously in case there's pending data to be written. This
// may happen when writing trailing headers.
if (code == NGHTTP2_NO_ERROR && nghttp2_session_want_write(handle) &&
!env->is_stopping()) {
env->can_call_into_js()) {
env->SetImmediate([handle, id, code, user_data](Environment* env) {
OnStreamClose(handle, id, code, user_data);
});
Expand Down
2 changes: 1 addition & 1 deletion src/stream_base.cc
Expand Up @@ -608,7 +608,7 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished(
StreamReq* req_wrap, int status) {
StreamBase* stream = static_cast<StreamBase*>(stream_);
Environment* env = stream->stream_env();
if (env->is_stopping()) return;
if (!env->can_call_into_js()) return;
AsyncWrap* async_wrap = req_wrap->GetAsyncWrap();
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
Expand Down

0 comments on commit ac5ee86

Please sign in to comment.