Skip to content

Commit

Permalink
Add temporary module logging (#2083)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jasnell
jasnell committed May 3, 2024
1 parent a105553 commit c3c30aa
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 29 deletions.
72 changes: 43 additions & 29 deletions src/workerd/jsg/modules.c++
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,47 @@
#include <set>

namespace workerd::jsg {

namespace {
// This list must be kept in sync with the list of builtins from Node.js.
// It should be unlikely that anything is ever removed from this list, and
// adding items to it is considered a semver-major change in Node.js.
static const std::set<kj::StringPtr> NODEJS_BUILTINS {
"_http_agent", "_http_client", "_http_common",
"_http_incoming", "_http_outgoing", "_http_server",
"_stream_duplex", "_stream_passthrough", "_stream_readable",
"_stream_transform", "_stream_wrap", "_stream_writable",
"_tls_common", "_tls_wrap", "assert",
"assert/strict", "async_hooks", "buffer",
"child_process", "cluster", "console",
"constants", "crypto", "dgram",
"diagnostics_channel", "dns", "dns/promises",
"domain", "events", "fs",
"fs/promises", "http", "http2",
"https", "inspector", "inspector/promises",
"module", "net", "os",
"path", "path/posix", "path/win32",
"perf_hooks", "process", "punycode",
"querystring", "readline", "readline/promises",
"repl", "stream", "stream/consumers",
"stream/promises", "stream/web", "string_decoder",
"sys", "timers", "timers/promises",
"tls", "trace_events", "tty",
"url", "util", "util/types",
"v8", "vm", "worker_threads",
"zlib"
};

} // namespace

void logIfNodeSpecifier(kj::StringPtr specifier) {
if (NODEJS_BUILTINS.contains(specifier)) {
LOG_NOSENTRY(WARNING, "Using require() to import a module with a known Node.js built-in name",
specifier);
}
}

namespace {
// The CompileCache is used to hold cached compilation data for built-in JavaScript modules.
//
// Importantly, this is a process-lifetime in-memory cache that is only appropriate for
Expand Down Expand Up @@ -58,6 +97,7 @@ v8::MaybeLocal<v8::Module> resolveCallback(v8::Local<v8::Context> context,
"referrer passed to resolveCallback isn't in modules table");

auto spec = kj::str(specifier);
logIfNodeSpecifier(spec);

// If the referrer module is a built-in, it is only permitted to resolve
// internal modules. If the worker bundle provided an override for a builtin,
Expand Down Expand Up @@ -254,6 +294,8 @@ v8::Local<v8::Value> CommonJsModuleContext::require(jsg::Lock& js, kj::String sp
auto modulesForResolveCallback = getModulesForResolveCallback(js.v8Isolate);
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");

logIfNodeSpecifier(specifier);

kj::Path targetPath = ([&] {
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
Expand Down Expand Up @@ -556,39 +598,11 @@ NodeJsModuleContext::NodeJsModuleContext(jsg::Lock& js, kj::Path path)
exports(js.v8Ref(module->getExports(js))) {}

v8::Local<v8::Value> NodeJsModuleContext::require(jsg::Lock& js, kj::String specifier) {
// This list must be kept in sync with the list of builtins from Node.js.
// It should be unlikely that anything is ever removed from this list, and
// adding items to it is considered a semver-major change in Node.js.
static const std::set<kj::StringPtr> NODEJS_BUILTINS {
"_http_agent", "_http_client", "_http_common",
"_http_incoming", "_http_outgoing", "_http_server",
"_stream_duplex", "_stream_passthrough", "_stream_readable",
"_stream_transform", "_stream_wrap", "_stream_writable",
"_tls_common", "_tls_wrap", "assert",
"assert/strict", "async_hooks", "buffer",
"child_process", "cluster", "console",
"constants", "crypto", "dgram",
"diagnostics_channel", "dns", "dns/promises",
"domain", "events", "fs",
"fs/promises", "http", "http2",
"https", "inspector", "inspector/promises",
"module", "net", "os",
"path", "path/posix", "path/win32",
"perf_hooks", "process", "punycode",
"querystring", "readline", "readline/promises",
"repl", "stream", "stream/consumers",
"stream/promises", "stream/web", "string_decoder",
"sys", "timers", "timers/promises",
"tls", "trace_events", "tty",
"url", "util", "util/types",
"v8", "vm", "worker_threads",
"zlib"
};

// If it is a bare specifier known to be a Node.js built-in, then prefix the
// specifier with node:
bool isNodeBuiltin = false;
auto resolveOption = jsg::ModuleRegistry::ResolveOption::DEFAULT;
logIfNodeSpecifier(specifier);
if (NODEJS_BUILTINS.contains(specifier)) {
specifier = kj::str("node:", specifier);
isNodeBuiltin = true;
Expand Down
11 changes: 11 additions & 0 deletions src/workerd/jsg/modules.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

namespace workerd::jsg {

void logIfNodeSpecifier(kj::StringPtr specifier);

class CommonJsModuleContext;

class CommonJsModuleObject: public jsg::Object {
Expand Down Expand Up @@ -434,6 +436,12 @@ class ModuleRegistryImpl final: public ModuleRegistry {

void add(kj::Path& specifier, ModuleInfo&& info) {
entries.insert(kj::heap<Entry>(specifier, Type::BUNDLE, kj::fwd<ModuleInfo>(info)));

if (entries.size() > 1) {
// This is just to give us some sense of how many workers make use of multiple modules
// in the bundle...
LOG_PERIODICALLY(WARNING, "NOSENTRY Worker using multiple modules in a single bundle.");
}
}

void addBuiltinModule(Module::Reader module) {
Expand Down Expand Up @@ -827,6 +835,9 @@ v8::MaybeLocal<v8::Promise> dynamicImportCallback(v8::Local<v8::Context> context
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
auto spec = kj::str(specifier);

logIfNodeSpecifier(spec);

if (spec.startsWith("node:") ||
spec.startsWith("cloudflare:") ||
spec.startsWith("workerd:")) {
Expand Down

0 comments on commit c3c30aa

Please sign in to comment.