Skip to content

Commit

Permalink
module: print location of unsettled top-level await in entry points
Browse files Browse the repository at this point in the history
When the entry point is a module and the graph it imports still
contains unsettled top-level await when the Node.js instance
finishes the event loop, search from the entry point module
for unsettled top-level await and print their location.

To avoid unnecessary overhead, we register a promise that only
gets settled when the entry point graph evaluation returns
from await, and only search the module graph if it's still
unsettled by the time the instance is exiting.

This patch only handles this for entry point modules. Other kinds of
modules are more complicated so will be left for the future.

Drive-by: update the terminology "unfinished promise" to the
more correct one "unsettled promise" in the codebase.

PR-URL: nodejs#51999
Fixes: nodejs#42868
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
  • Loading branch information
joyeecheung authored and jcbhmr committed May 15, 2024
1 parent 0b94456 commit 12224ed
Show file tree
Hide file tree
Showing 16 changed files with 192 additions and 36 deletions.
4 changes: 2 additions & 2 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -3953,8 +3953,8 @@ cases:
and generally can only happen during development of Node.js itself.
* `12` **Invalid Debug Argument**: The `--inspect` and/or `--inspect-brk`
options were set, but the port number chosen was invalid or unavailable.
* `13` **Unfinished Top-Level Await**: `await` was used outside of a function
in the top-level code, but the passed `Promise` never resolved.
* `13` **Unsettled Top-Level Await**: `await` was used outside of a function
in the top-level code, but the passed `Promise` never settled.
* `14` **Snapshot Failure**: Node.js was started to build a V8 startup
snapshot and it failed because certain requirements of the state of
the application were not met.
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const {
ERR_METHOD_NOT_IMPLEMENTED,
ERR_WORKER_UNSERIALIZABLE_ERROR,
} = require('internal/errors').codes;
const { exitCodes: { kUnfinishedTopLevelAwait } } = internalBinding('errors');
const { exitCodes: { kUnsettledTopLevelAwait } } = internalBinding('errors');
const { URL } = require('internal/url');
const { canParse: URLCanParse } = internalBinding('url');
const { receiveMessageOnPort } = require('worker_threads');
Expand Down Expand Up @@ -615,7 +615,7 @@ class HooksProxy {
} while (response == null);
debug('got sync response from worker', { method, args });
if (response.message.status === 'never-settle') {
process.exit(kUnfinishedTopLevelAwait);
process.exit(kUnsettledTopLevelAwait);
} else if (response.message.status === 'exit') {
process.exit(response.message.body);
}
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class ModuleLoader {
}
}

async eval(source, url) {
async eval(source, url, isEntryPoint = false) {
const evalInstance = (url) => {
const { ModuleWrap } = internalBinding('module_wrap');
const { registerModule } = require('internal/modules/esm/utils');
Expand All @@ -201,7 +201,7 @@ class ModuleLoader {
const job = new ModuleJob(
this, url, undefined, evalInstance, false, false);
this.loadCache.set(url, undefined, job);
const { module } = await job.run();
const { module } = await job.run(isEntryPoint);

return {
__proto__: null,
Expand Down Expand Up @@ -311,9 +311,9 @@ class ModuleLoader {
* module import.
* @returns {Promise<ModuleExports>}
*/
async import(specifier, parentURL, importAttributes) {
async import(specifier, parentURL, importAttributes, isEntryPoint = false) {
const moduleJob = await this.getModuleJob(specifier, parentURL, importAttributes);
const { module } = await moduleJob.run();
const { module } = await moduleJob.run(isEntryPoint);
return module.getNamespace();
}

Expand Down
12 changes: 10 additions & 2 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ const {
StringPrototypeIncludes,
StringPrototypeSplit,
StringPrototypeStartsWith,
globalThis,
} = primordials;

const { ModuleWrap } = internalBinding('module_wrap');

const {
privateSymbols: {
entry_point_module_private_symbol,
},
} = internalBinding('util');
const { decorateErrorStack, kEmptyObject } = require('internal/util');
const {
getSourceMapsEnabled,
Expand Down Expand Up @@ -213,8 +218,11 @@ class ModuleJob {
return { __proto__: null, module: this.module };
}

async run() {
async run(isEntryPoint = false) {
await this.instantiate();
if (isEntryPoint) {
globalThis[entry_point_module_private_symbol] = this.module;
}
const timeout = -1;
const breakOnSigint = false;
setHasStartedUserESMExecution();
Expand Down
27 changes: 11 additions & 16 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const {
StringPrototypeEndsWith,
globalThis,
} = primordials;

const { containsModuleSyntax } = internalBinding('contextify');
Expand All @@ -16,9 +17,12 @@ const {
} = require('internal/process/execution');
const {
triggerUncaughtException,
exitCodes: { kUnfinishedTopLevelAwait },
} = internalBinding('errors');

const {
privateSymbols: {
entry_point_promise_private_symbol,
},
} = internalBinding('util');
/**
* Get the absolute path to the main entry point.
* @param {string} main - Entry point path
Expand Down Expand Up @@ -102,20 +106,10 @@ function shouldUseESMLoader(mainPath) {
return type === 'module';
}

/**
* Handle a Promise from running code that potentially does Top-Level Await.
* In that case, it makes sense to set the exit code to a specific non-zero value
* if the main code never finishes running.
*/
function handleProcessExit() {
process.exitCode ??= kUnfinishedTopLevelAwait;
}

/**
* @param {function(ModuleLoader):ModuleWrap|undefined} callback
*/
async function asyncRunEntryPointWithESMLoader(callback) {
process.on('exit', handleProcessExit);
const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader();
try {
const userImports = getOptionValue('--import');
Expand All @@ -137,8 +131,6 @@ async function asyncRunEntryPointWithESMLoader(callback) {
err,
true, /* fromPromise */
);
} finally {
process.off('exit', handleProcessExit);
}
}

Expand All @@ -152,6 +144,10 @@ async function asyncRunEntryPointWithESMLoader(callback) {
*/
function runEntryPointWithESMLoader(callback) {
const promise = asyncRunEntryPointWithESMLoader(callback);
// Register the promise - if by the time the event loop finishes running, this is
// still unsettled, we'll search the graph from the entry point module and print
// the location of any unsettled top-level await found.
globalThis[entry_point_promise_private_symbol] = promise;
return promise;
}

Expand All @@ -171,7 +167,7 @@ function executeUserEntryPoint(main = process.argv[1]) {
const mainURL = pathToFileURL(mainPath).href;

runEntryPointWithESMLoader((cascadedLoader) => {
// Note that if the graph contains unfinished TLA, this may never resolve
// Note that if the graph contains unsettled TLA, this may never resolve
// even after the event loop stops running.
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true);
});
Expand All @@ -185,5 +181,4 @@ function executeUserEntryPoint(main = process.argv[1]) {
module.exports = {
executeUserEntryPoint,
runEntryPointWithESMLoader,
handleProcessExit,
};
3 changes: 0 additions & 3 deletions lib/internal/process/per_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,6 @@ function wrapProcessMethods(binding) {
memoryUsage.rss = rss;

function exit(code) {
const { handleProcessExit } = require('internal/modules/run_main');
process.off('exit', handleProcessExit);

if (arguments.length !== 0) {
process.exitCode = code;
}
Expand Down
15 changes: 14 additions & 1 deletion src/api/embed_helpers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,20 @@ Maybe<ExitCode> SpinEventLoopInternal(Environment* env) {

env->PrintInfoForSnapshotIfDebug();
env->ForEachRealm([](Realm* realm) { realm->VerifyNoStrongBaseObjects(); });
return EmitProcessExitInternal(env);
Maybe<ExitCode> exit_code = EmitProcessExitInternal(env);
if (exit_code.FromMaybe(ExitCode::kGenericUserError) !=
ExitCode::kNoFailure) {
return exit_code;
}

auto unsettled_tla = env->CheckUnsettledTopLevelAwait();
if (unsettled_tla.IsNothing()) {
return Nothing<ExitCode>();
}
if (!unsettled_tla.FromJust()) {
return Just(ExitCode::kUnsettledTopLevelAwait);
}
return Just(ExitCode::kNoFailure);
}

struct CommonEnvironmentSetup::Impl {
Expand Down
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,10 @@ inline void Environment::set_exiting(bool value) {
exit_info_[kExiting] = value ? 1 : 0;
}

inline bool Environment::exiting() const {
return exit_info_[kExiting] == 1;
}

inline ExitCode Environment::exit_code(const ExitCode default_code) const {
return exit_info_[kHasExitCode] == 0
? default_code
Expand Down
37 changes: 37 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "debug_utils-inl.h"
#include "diagnosticfilename-inl.h"
#include "memory_tracker-inl.h"
#include "module_wrap.h"
#include "node_buffer.h"
#include "node_context_data.h"
#include "node_contextify.h"
Expand Down Expand Up @@ -50,6 +51,7 @@ using v8::HeapSpaceStatistics;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Number;
Expand Down Expand Up @@ -1228,6 +1230,41 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
at_exit_functions_.push_front(ExitCallback{cb, arg});
}

Maybe<bool> Environment::CheckUnsettledTopLevelAwait() {
HandleScope scope(isolate_);
Local<Context> ctx = context();
Local<Value> value;

Local<Value> entry_point_promise;
if (!ctx->Global()
->GetPrivate(ctx, entry_point_promise_private_symbol())
.ToLocal(&entry_point_promise)) {
return v8::Nothing<bool>();
}
if (!entry_point_promise->IsPromise()) {
return v8::Just(true);
}
if (entry_point_promise.As<Promise>()->State() !=
Promise::PromiseState::kPending) {
return v8::Just(true);
}

if (!ctx->Global()
->GetPrivate(ctx, entry_point_module_private_symbol())
.ToLocal(&value)) {
return v8::Nothing<bool>();
}
if (!value->IsObject()) {
return v8::Just(true);
}
Local<Object> object = value.As<Object>();
CHECK(BaseObject::IsBaseObject(isolate_data_, object));
CHECK_EQ(object->InternalFieldCount(),
loader::ModuleWrap::kInternalFieldCount);
auto* wrap = BaseObject::FromJSObject<loader::ModuleWrap>(object);
return wrap->CheckUnsettledTopLevelAwait();
}

void Environment::RunAndClearInterrupts() {
while (native_immediates_interrupts_.size() > 0) {
NativeImmediateQueue queue;
Expand Down
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,7 @@ class Environment : public MemoryRetainer {
// a pseudo-boolean to indicate whether the exit code is undefined.
inline AliasedInt32Array& exit_info();
inline void set_exiting(bool value);
bool exiting() const;
inline ExitCode exit_code(const ExitCode default_code) const;

// This stores whether the --abort-on-uncaught-exception flag was passed
Expand Down Expand Up @@ -840,6 +841,7 @@ class Environment : public MemoryRetainer {
void AtExit(void (*cb)(void* arg), void* arg);
void RunAtExitCallbacks();

v8::Maybe<bool> CheckUnsettledTopLevelAwait();
void RunWeakRefCleanup();

v8::MaybeLocal<v8::Value> RunSnapshotSerializeCallback() const;
Expand Down
2 changes: 2 additions & 0 deletions src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
V(transfer_mode_private_symbol, "node:transfer_mode") \
V(host_defined_option_symbol, "node:host_defined_option_symbol") \
V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \
V(entry_point_module_private_symbol, "node:entry_point_module") \
V(entry_point_promise_private_symbol, "node:entry_point_promise") \
V(napi_type_tag, "node:napi:type_tag") \
V(napi_wrapper, "node:napi:wrapper") \
V(untransferable_object_private_symbol, "node:untransferableObject") \
Expand Down
38 changes: 38 additions & 0 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,44 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env,
return nullptr;
}

v8::Maybe<bool> ModuleWrap::CheckUnsettledTopLevelAwait() {
Isolate* isolate = env()->isolate();
Local<Context> context = env()->context();

// This must be invoked when the environment is shutting down, and the module
// is kept alive by the module wrap via an internal field.
CHECK(env()->exiting());
CHECK(!module_.IsEmpty());

Local<Module> module = module_.Get(isolate);
// It's a synthetic module, likely a facade wrapping CJS.
if (!module->IsSourceTextModule()) {
return v8::Just(true);
}

if (!module->IsGraphAsync()) { // There is no TLA, no need to check.
return v8::Just(true);
}
auto stalled = module->GetStalledTopLevelAwaitMessage(isolate);
if (stalled.size() == 0) {
return v8::Just(true);
}

if (env()->options()->warnings) {
for (auto pair : stalled) {
Local<v8::Message> message = std::get<1>(pair);

std::string reason = "Warning: Detected unsettled top-level await at ";
std::string info =
FormatErrorMessage(isolate, context, "", message, true);
reason += info;
FPrintF(stderr, "%s\n", reason);
}
}

return v8::Just(false);
}

// new ModuleWrap(url, context, source, lineOffset, columnOffset)
// new ModuleWrap(url, context, exportNames, syntheticExecutionFunction)
void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
Expand Down
1 change: 1 addition & 0 deletions src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class ModuleWrap : public BaseObject {
}

v8::Local<v8::Context> context() const;
v8::Maybe<bool> CheckUnsettledTopLevelAwait();

SET_MEMORY_INFO_NAME(ModuleWrap)
SET_SELF_SIZE(ModuleWrap)
Expand Down
2 changes: 1 addition & 1 deletion src/node_exit_code.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace node {
/* This was intended for invalid inspector arguments but is actually now */ \
/* just a duplicate of InvalidCommandLineArgument */ \
V(InvalidCommandLineArgument2, 12) \
V(UnfinishedTopLevelAwait, 13) \
V(UnsettledTopLevelAwait, 13) \
V(StartupSnapshotFailure, 14) \
/* If the process exits from unhandled signals e.g. SIGABRT, SIGTRAP, */ \
/* typically the exit codes are 128 + signal number. We also exit with */ \
Expand Down
Loading

0 comments on commit 12224ed

Please sign in to comment.