Skip to content

Commit

Permalink
Revert "embedding: make Stop() stop Workers"
Browse files Browse the repository at this point in the history
This reverts commit 037ac99.

As flaky CI failures have revealed, this feature was implemented
incorrectly. `stop_sub_worker_contexts()` needs to be called on the
thread on which the `Environment` is currently running, it’s not
thread-safe. The current API requires `Stop()` to be thread-safe,
though.

We could add a new API for this, but unless there’s demand, that’s
probably not necessary as `FreeEnvironment()` will also stop Workers,
which is commonly the next action on an `Environment` instance after
having `Stop()` called on it.

Refs: #32531

PR-URL: #32623
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
  • Loading branch information
addaleax authored and BethGriggs committed Apr 7, 2020
1 parent 6fca565 commit b5aaf23
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 9 deletions.
3 changes: 2 additions & 1 deletion src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,8 @@ ThreadId AllocateEnvironmentThreadId() {
}

void DefaultProcessExitHandler(Environment* env, int exit_code) {
Stop(env);
env->set_can_call_into_js(false);
env->stop_sub_worker_contexts();
DisposePlatform();
exit(exit_code);
}
Expand Down
3 changes: 1 addition & 2 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -528,10 +528,9 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
}
}

void Environment::Stop() {
void Environment::ExitEnv() {
set_can_call_into_js(false);
set_stopping(true);
stop_sub_worker_contexts();
isolate_->TerminateExecution();
SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); });
}
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ class Environment : public MemoryRetainer {
void RegisterHandleCleanups();
void CleanupHandles();
void Exit(int code);
void Stop();
void ExitEnv();

// Register clean-up cb to be called on environment destruction.
inline void RegisterHandleCleanup(uv_handle_t* handle,
Expand Down
2 changes: 1 addition & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1059,7 +1059,7 @@ int Start(int argc, char** argv) {
}

int Stop(Environment* env) {
env->Stop();
env->ExitEnv();
return 0;
}

Expand Down
7 changes: 3 additions & 4 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,7 @@ class Environment;
NODE_EXTERN int Start(int argc, char* argv[]);

// Tear down Node.js while it is running (there are active handles
// in the loop and / or actively executing JavaScript code). This also stops
// all Workers that may have been started earlier.
// in the loop and / or actively executing JavaScript code).
NODE_EXTERN int Stop(Environment* env);

// TODO(addaleax): Officially deprecate this and replace it with something
Expand Down Expand Up @@ -479,8 +478,8 @@ NODE_EXTERN void FreeEnvironment(Environment* env);
// It receives the Environment* instance and the exit code as arguments.
// This could e.g. call Stop(env); in order to terminate execution and stop
// the event loop.
// The default handler calls Stop(), disposes of the global V8 platform
// instance, if one is being used, and calls exit().
// The default handler disposes of the global V8 platform instance, if one is
// being used, and calls exit().
NODE_EXTERN void SetProcessExitHandler(
Environment* env,
std::function<void(Environment*, int)>&& handler);
Expand Down

0 comments on commit b5aaf23

Please sign in to comment.