Skip to content

Commit ac5ee86

Browse files
committed
src: distinguish env stopping flags
`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_`.
1 parent 1d6fda0 commit ac5ee86

File tree

8 files changed

+18
-12
lines changed

8 files changed

+18
-12
lines changed

src/api/environment.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,7 @@ bool AllowWasmCodeGenerationCallback(Local<Context> context,
5252
bool ShouldAbortOnUncaughtException(Isolate* isolate) {
5353
DebugSealHandleScope scope(isolate);
5454
Environment* env = Environment::GetCurrent(isolate);
55-
return env != nullptr &&
56-
(env->is_main_thread() || !env->is_stopping()) &&
55+
return env != nullptr && (env->is_main_thread() || env->can_call_into_js()) &&
5756
env->abort_on_uncaught_exception() &&
5857
env->should_abort_on_uncaught_toggle()[0] &&
5958
!env->inside_should_not_abort_on_uncaught_scope();
@@ -404,6 +403,9 @@ void FreeEnvironment(Environment* env) {
404403
Context::Scope context_scope(env->context());
405404
SealHandleScope seal_handle_scope(isolate);
406405

406+
// Set the flag in accordance with the DisallowJavascriptExecutionScope
407+
// above.
408+
env->set_can_call_into_js(false);
407409
env->set_stopping(true);
408410
env->stop_sub_worker_contexts();
409411
env->RunCleanup();

src/env-inl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -613,7 +613,7 @@ void Environment::RequestInterrupt(Fn&& cb) {
613613
}
614614

615615
inline bool Environment::can_call_into_js() const {
616-
return can_call_into_js_ && !is_stopping();
616+
return can_call_into_js_;
617617
}
618618

619619
inline void Environment::set_can_call_into_js(bool can_call_into_js) {

src/env.cc

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,7 @@ Environment::~Environment() {
809809
}
810810

811811
// FreeEnvironment() should have set this.
812-
CHECK(is_stopping());
812+
CHECK(!can_call_into_js());
813813

814814
if (heapsnapshot_near_heap_limit_callback_added_) {
815815
RemoveHeapSnapshotNearHeapLimitCallback(0);
@@ -905,10 +905,13 @@ void Environment::InitializeLibuv() {
905905
}
906906

907907
void Environment::ExitEnv() {
908-
set_can_call_into_js(false);
908+
// Should not access non-thread-safe methods here.
909909
set_stopping(true);
910910
isolate_->TerminateExecution();
911-
SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); });
911+
SetImmediateThreadsafe([](Environment* env) {
912+
env->set_can_call_into_js(false);
913+
uv_stop(env->event_loop());
914+
});
912915
}
913916

914917
void Environment::RegisterHandleCleanups() {

src/env.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,9 @@ class Environment : public MemoryRetainer {
781781
void stop_sub_worker_contexts();
782782
template <typename Fn>
783783
inline void ForEachWorker(Fn&& iterator);
784+
// Determine if the environment is stopping. This getter is thread-safe.
785+
// If the check is not performed off thread, the flag
786+
// `env->can_call_into_js()` should be preferred.
784787
inline bool is_stopping() const;
785788
inline void set_stopping(bool value);
786789
inline std::list<node_module>* extra_linked_bindings();

src/module_wrap.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -410,8 +410,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
410410

411411
// Convert the termination exception into a regular exception.
412412
if (timed_out || received_signal) {
413-
if (!env->is_main_thread() && env->is_stopping())
414-
return;
413+
if (!env->is_main_thread() && !env->can_call_into_js()) return;
415414
env->isolate()->CancelTerminateExecution();
416415
// It is possible that execution was terminated by another timeout in
417416
// which this timeout is nested, so check whether one of the watchdogs

src/node_contextify.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,8 +1028,7 @@ bool ContextifyScript::EvalMachine(Local<Context> context,
10281028

10291029
// Convert the termination exception into a regular exception.
10301030
if (timed_out || received_signal) {
1031-
if (!env->is_main_thread() && env->is_stopping())
1032-
return false;
1031+
if (!env->is_main_thread() && !env->can_call_into_js()) return false;
10331032
env->isolate()->CancelTerminateExecution();
10341033
// It is possible that execution was terminated by another timeout in
10351034
// which this timeout is nested, so check whether one of the watchdogs

src/node_http2.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1127,7 +1127,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
11271127
// Don't close synchronously in case there's pending data to be written. This
11281128
// may happen when writing trailing headers.
11291129
if (code == NGHTTP2_NO_ERROR && nghttp2_session_want_write(handle) &&
1130-
!env->is_stopping()) {
1130+
env->can_call_into_js()) {
11311131
env->SetImmediate([handle, id, code, user_data](Environment* env) {
11321132
OnStreamClose(handle, id, code, user_data);
11331133
});

src/stream_base.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,7 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished(
608608
StreamReq* req_wrap, int status) {
609609
StreamBase* stream = static_cast<StreamBase*>(stream_);
610610
Environment* env = stream->stream_env();
611-
if (env->is_stopping()) return;
611+
if (!env->can_call_into_js()) return;
612612
AsyncWrap* async_wrap = req_wrap->GetAsyncWrap();
613613
HandleScope handle_scope(env->isolate());
614614
Context::Scope context_scope(env->context());

0 commit comments

Comments
 (0)