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.

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

Node SEA with useSnapshot cannot start worker thread #56077

Closed
nomagick opened this issue Nov 29, 2024 · 3 comments · Fixed by #56120
Closed

Node SEA with useSnapshot cannot start worker thread #56077

nomagick opened this issue Nov 29, 2024 · 3 comments · Fixed by #56120
Labels
single-executable Issues and PRs related to single-executable applications snapshot Issues and PRs related to the startup snapshot

Comments

@nomagick
Copy link

Version

v22.11.0

Platform

Darwin Mac.lan 24.1.0 Darwin Kernel Version 24.1.0: Thu Oct 10 21:03:11 PDT 2024; root:xnu-11215.41.3~2/RELEASE_ARM64_T6020 arm64

Subsystem

sea

What steps will reproduce the bug?

Generate a SEA and start a worker thread in it

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Thread can be started.

What do you see instead?

Native stack trace

Additional information


  #  ./out.bin[76089]: MaybeLocal<v8::Value> node::StartExecution(node::Environment *, node::StartExecutionCallback) at ../src/node.cc:378
  #  Assertion failed: !(sea.use_snapshot()) || (!env->snapshot_deserialize_main().IsEmpty())

----- Native stack trace -----

 1: 0x1006b3778 node::Assert(node::AssertionInfo const&) [/Users/yanlong.wang/out.bin]
 2: 0x1022b7664 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) (.cold.1) [/Users/yanlong.wang/out.bin]
 3: 0x10066dc04 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) [/Users/yanlong.wang/out.bin]
 4: 0x1005d6460 node::LoadEnvironment(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>, std::__1::function<void (node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>)>) [/Users/yanlong.wang/out.bin]
 5: 0x1007a48bc node::worker::Worker::Run() [/Users/yanlong.wang/out.bin]
 6: 0x1007a8b0c node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::$_3::__invoke(void*) [/Users/yanlong.wang/out.bin]
 7: 0x192a832e4 _pthread_start [/usr/lib/system/libsystem_pthread.dylib]
 8: 0x192a7e0fc thread_start [/usr/lib/system/libsystem_pthread.dylib]
[1]    76089 abort      ./out.bin
@nomagick
Copy link
Author

This also breaks the esm loader hooks because it also runs in a thread.

@nomagick
Copy link
Author

Looks like here:

node/src/node.cc

Lines 322 to 350 in 03ec900

#ifndef DISABLE_SINGLE_EXECUTABLE_APPLICATION
if (sea::IsSingleExecutable()) {
sea::SeaResource sea = sea::FindSingleExecutableResource();
// The SEA preparation blob building process should already enforce this,
// this check is just here to guard against the unlikely case where
// the SEA preparation blob has been manually modified by someone.
CHECK_IMPLIES(sea.use_snapshot(),
!env->snapshot_deserialize_main().IsEmpty());
}
#endif
// Ignore env file if we're in watch mode.
// Without it env is not updated when restarting child process.
// Child process has --watch flag removed, so it will load the file.
if (env->options()->has_env_file_string && !env->options()->watch_mode) {
per_process::dotenv_file.SetEnvironment(env);
}
// TODO(joyeecheung): move these conditions into JS land and let the
// deserialize main function take precedence. For workers, we need to
// move the pre-execution part into a different file that can be
// reused when dealing with user-defined main functions.
if (!env->snapshot_deserialize_main().IsEmpty()) {
return env->RunSnapshotDeserializeMain();
}
if (env->worker_context() != nullptr) {
return StartExecution(env, "internal/main/worker_thread");
}

@VoltrexKeyva VoltrexKeyva added the single-executable Issues and PRs related to single-executable applications label Dec 2, 2024
@joyeecheung
Copy link
Member

Hmm, looks like we just need to skip the SEA stuff for worker threads during bootstrap, since that's meant for the main thread anyway.

@joyeecheung joyeecheung added the snapshot Issues and PRs related to the startup snapshot label Dec 3, 2024
nodejs-github-bot pushed a commit that referenced this issue Dec 7, 2024
Snapshot main functions are only loaded for main threads in single
executable applications. Update the check to avoid asserting it
in worker threads - this allows worker threads to be spawned in
snapshot main functions bundled into a single executable
application.

PR-URL: #56120
Fixes: #56077
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this issue Dec 10, 2024
Snapshot main functions are only loaded for main threads in single
executable applications. Update the check to avoid asserting it
in worker threads - this allows worker threads to be spawned in
snapshot main functions bundled into a single executable
application.

PR-URL: #56120
Fixes: #56077
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
ruyadorno pushed a commit that referenced this issue Dec 20, 2024
Snapshot main functions are only loaded for main threads in single
executable applications. Update the check to avoid asserting it
in worker threads - this allows worker threads to be spawned in
snapshot main functions bundled into a single executable
application.

PR-URL: #56120
Fixes: #56077
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
single-executable Issues and PRs related to single-executable applications snapshot Issues and PRs related to the startup snapshot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants