simplify options

Author: guybedford

src/workerd/api/node/tests/process-stdio-nodejs-test.wd-test

@@ -14,5 +14,5 @@ const unitTests :Workerd.Config = (
         ],
       )
     ),
-  ]
+  ],
 );

src/workerd/io/bundle-fs-test.c++

@@ -116,8 +116,7 @@ KJ_TEST("Guarding against circular symlinks works") {
   fixture.runInIoContext([&](const TestFixture::Environment& env) {
     // We don't need to set up a TmpDirStorageScope since we are running
     // within a test fixture that sets up an IoContext...
-    auto vfs =
-        newVirtualFileSystem(kj::heap<FsMap>(), getTmpDirectoryImpl(), ProcessStdioPrefixed::YES);
+    auto vfs = newVirtualFileSystem(kj::heap<FsMap>(), getTmpDirectoryImpl());
 
     // Set up circular symlinks
     auto maybeTemp = KJ_ASSERT_NONNULL(vfs->resolve(env.js, "file:///"_url));

src/workerd/io/worker-fs.c++

@@ -895,14 +895,11 @@ class FdHandle final {
 
 class VirtualFileSystemImpl final: public VirtualFileSystem {
  public:
-  VirtualFileSystemImpl(kj::Own<FsMap> fsMap,
-      kj::Rc<Directory>&& root,
-      kj::Own<VirtualFileSystem::Observer> observer,
-      ProcessStdioPrefixed processStdioPrefixed)
+  VirtualFileSystemImpl(
+      kj::Own<FsMap> fsMap, kj::Rc<Directory>&& root, kj::Own<VirtualFileSystem::Observer> observer)
       : fsMap(kj::mv(fsMap)),
         root(kj::mv(root)),
         observer(kj::mv(observer)),
-        processStdioPrefixed(processStdioPrefixed),
         weakThis(
             kj::rc<WeakRef<VirtualFileSystemImpl>>(kj::Badge<VirtualFileSystemImpl>(), *this)) {}
 
@@ -1120,7 +1117,6 @@ class VirtualFileSystemImpl final: public VirtualFileSystem {
   kj::Own<FsMap> fsMap;
   mutable kj::Rc<Directory> root;
   kj::Own<VirtualFileSystem::Observer> observer;
-  ProcessStdioPrefixed processStdioPrefixed;
   mutable kj::Rc<WeakRef<VirtualFileSystemImpl>> weakThis;
   friend class FdHandle;
 
@@ -1266,25 +1262,20 @@ kj::Rc<File> File::newReadable(kj::ArrayPtr<const kj::byte> data) {
   return kj::rc<FileImpl>(data);
 }
 
-kj::Own<VirtualFileSystem> newVirtualFileSystem(kj::Own<FsMap> fsMap,
-    kj::Rc<Directory>&& root,
-    kj::Own<VirtualFileSystem::Observer> observer,
-    ProcessStdioPrefixed processStdioPrefixed) {
-  return kj::heap<VirtualFileSystemImpl>(
-      kj::mv(fsMap), kj::mv(root), kj::mv(observer), processStdioPrefixed);
+kj::Own<VirtualFileSystem> newVirtualFileSystem(
+    kj::Own<FsMap> fsMap, kj::Rc<Directory>&& root, kj::Own<VirtualFileSystem::Observer> observer) {
+  return kj::heap<VirtualFileSystemImpl>(kj::mv(fsMap), kj::mv(root), kj::mv(observer));
 }
 
 kj::Own<VirtualFileSystem> newWorkerFileSystem(kj::Own<FsMap> fsMap,
     kj::Rc<Directory> bundleDirectory,
-    kj::Own<VirtualFileSystem::Observer> observer,
-    ProcessStdioPrefixed processStdioPrefixed) {
+    kj::Own<VirtualFileSystem::Observer> observer) {
   // Our root directory is a read-only directory
   Directory::Builder builder;
   builder.addPath(fsMap->getBundlePath(), kj::mv(bundleDirectory));
   builder.addPath(fsMap->getTempPath(), getTmpDirectoryImpl());
   builder.addPath(fsMap->getDevPath(), getDevDirectory());
-  return newVirtualFileSystem(
-      kj::mv(fsMap), builder.finish(), kj::mv(observer), processStdioPrefixed);
+  return newVirtualFileSystem(kj::mv(fsMap), builder.finish(), kj::mv(observer));
 }
 
 kj::Rc<Directory> getTmpDirectoryImpl() {
@@ -1761,10 +1752,7 @@ class DevRandomFile final: public File, public kj::EnableAddRefToThis<DevRandomF
 // - inspector reporting
 // - structured logging
 // - stdio output otherwise
-void writeStdio(jsg::Lock& js,
-    VirtualFileSystem::Stdio type,
-    kj::ArrayPtr<const kj::byte> bytes,
-    ProcessStdioPrefixed processStdioPrefixed) {
+void writeStdio(jsg::Lock& js, VirtualFileSystem::Stdio type, kj::ArrayPtr<const kj::byte> bytes) {
   auto chars = bytes.asChars();
   size_t endPos = chars.size();
   if (chars[endPos - 1] == '\n') endPos--;
@@ -1776,6 +1764,7 @@ void writeStdio(jsg::Lock& js,
       auto methodFunc = methodVal.As<v8::Function>();
 
       kj::String outputStr;
+      auto processStdioPrefixed = Worker::Isolate::from(js).getProcessStdioPrefixed();
       if (processStdioPrefixed == ProcessStdioPrefixed::YES) {
         if (endPos == 0) {
           v8::Local<v8::Value> args[] = {
@@ -1800,9 +1789,8 @@ void writeStdio(jsg::Lock& js,
 // logging mechanisms. Reads always return EOF (0-byte reads).
 class StdioFile final: public File, public kj::EnableAddRefToThis<StdioFile> {
  public:
-  StdioFile(VirtualFileSystem::Stdio type, ProcessStdioPrefixed processStdioPrefixed)
+  StdioFile(VirtualFileSystem::Stdio type)
       : type(type),
-        processStdioPrefixed(processStdioPrefixed),
         // TODO(sometime): Investigate if we can refactor out the weakref here?
         weakThis(kj::rc<WeakRef<StdioFile>>(kj::Badge<StdioFile>(), *this)) {}
 
@@ -1876,11 +1864,11 @@ class StdioFile final: public File, public kj::EnableAddRefToThis<StdioFile> {
           if (lineBuffer.size() > 0) {
             // We have buffered data - append the line data to it
             lineBuffer.addAll(lineData);
-            writeStdio(js, type, lineBuffer.asPtr(), processStdioPrefixed);
+            writeStdio(js, type, lineBuffer.asPtr());
             lineBuffer.clear();
           } else {
             // No buffered data - log this line directly
-            writeStdio(js, type, lineData, processStdioPrefixed);
+            writeStdio(js, type, lineData);
           }
 
           pos = newlinePos + 1;
@@ -1934,7 +1922,6 @@ class StdioFile final: public File, public kj::EnableAddRefToThis<StdioFile> {
 
  private:
   VirtualFileSystem::Stdio type;
-  ProcessStdioPrefixed processStdioPrefixed;
   mutable kj::Maybe<kj::String> maybeUniqueId;
 
   static constexpr size_t MAX_LINE_BUFFER_SIZE = 4096;
@@ -1956,7 +1943,7 @@ class StdioFile final: public File, public kj::EnableAddRefToThis<StdioFile> {
 
         if (self.lineBuffer.size() > 0) {
           if (IoContext::hasCurrent()) {
-            writeStdio(js, self.type, self.lineBuffer.asPtr(), self.processStdioPrefixed);
+            writeStdio(js, self.type, self.lineBuffer.asPtr());
           }
           self.lineBuffer.clear();
         }
@@ -1999,7 +1986,7 @@ kj::Rc<VirtualFileSystem::OpenedFile> VirtualFileSystemImpl::getStdio(
   KJ_IF_SOME(existing, openedFiles.find(n)) {
     return existing.addRef();
   }
-  auto stdioFile = kj::rc<StdioFile>(stdio, processStdioPrefixed);
+  auto stdioFile = kj::rc<StdioFile>(stdio);
   kj::Rc<workerd::File> file = kj::mv(stdioFile);
   auto opened = kj::rc<VirtualFileSystem::OpenedFile>(n, true, true, true, kj::mv(file));
   openedFiles.insert(n, opened.addRef());

src/workerd/io/worker-fs.h

@@ -694,8 +694,7 @@ class VirtualFileSystem {
 
 kj::Own<VirtualFileSystem> newVirtualFileSystem(kj::Own<FsMap> fsMap,
     kj::Rc<Directory>&& root,
-    kj::Own<VirtualFileSystem::Observer> observer,
-    workerd::ProcessStdioPrefixed processStdioPrefixed) KJ_WARN_UNUSED_RESULT;
+    kj::Own<VirtualFileSystem::Observer> observer) KJ_WARN_UNUSED_RESULT;
 
 // The FsMap is a configurable mapping of built-in "known" file system
 // paths to user-configurable locations. It is used to allow user-specified
@@ -803,8 +802,7 @@ class SymbolicLinkRecursionGuardScope final {
 // provides the directory structure for the worker's bundle.
 kj::Own<VirtualFileSystem> newWorkerFileSystem(kj::Own<FsMap> fsMap,
     kj::Rc<Directory> bundleDirectory,
-    kj::Own<VirtualFileSystem::Observer> observer,
-    workerd::ProcessStdioPrefixed processStdioPrefixed) KJ_WARN_UNUSED_RESULT;
+    kj::Own<VirtualFileSystem::Observer> observer) KJ_WARN_UNUSED_RESULT;
 
 // Exposed only for testing purposes.
 kj::Rc<Directory> getTmpDirectoryImpl() KJ_WARN_UNUSED_RESULT;

src/workerd/io/worker.c++

@@ -575,8 +575,7 @@ struct Worker::Isolate::Impl {
           progressCounter(impl.lockSuccessCount),
           oldCurrentApi(currentApi),
           limitEnforcer(isolate.getLimitEnforcer()),
-          consoleMode(isolate.consoleMode),
-          structuredLogging(isolate.structuredLogging),
+          loggingOptions(isolate.loggingOptions),
           lock(isolate.api->lock(stackScope)) {
       WarnAboutIsolateLockScope::maybeWarn();
 
@@ -624,7 +623,7 @@ struct Worker::Isolate::Impl {
         i.get()->contextCreated(
             v8_inspector::V8ContextInfo(context, 1, jsg::toInspectorStringView("Worker")));
       }
-      Worker::setupContext(*lock, context, consoleMode, structuredLogging);
+      Worker::setupContext(*lock, context, loggingOptions);
     }
 
     void disposeContext(jsg::JsContext<api::ServiceWorkerGlobalScope> context) {
@@ -659,11 +658,9 @@ struct Worker::Isolate::Impl {
 
     const IsolateLimitEnforcer& limitEnforcer;  // only so we can call getIsolateStats()
 
-    ConsoleMode consoleMode;
-
     // When structuredLogging is YES AND consoleMode is STDOUT js logs will be emitted to STDOUT
-    // as newline separated json objects.
-    StructuredLogging structuredLogging;
+    // as newline separated json objects
+    LoggingOptions loggingOptions;
 
    public:
     kj::Own<jsg::Lock> lock;
@@ -1019,16 +1016,12 @@ Worker::Isolate::Isolate(kj::Own<Api> apiParam,
     kj::StringPtr id,
     kj::Own<IsolateLimitEnforcer> limitEnforcerParam,
     InspectorPolicy inspectorPolicy,
-    ConsoleMode consoleMode,
-    StructuredLogging structuredLogging,
-    ProcessStdioPrefixed processStdioPrefixed)
+    LoggingOptions loggingOptions)
     : metrics(kj::mv(metricsParam)),
       id(kj::str(id)),
       limitEnforcer(kj::mv(limitEnforcerParam)),
       api(kj::mv(apiParam)),
-      consoleMode(consoleMode),
-      structuredLogging(structuredLogging),
-      processStdioPrefixed(processStdioPrefixed),
+      loggingOptions(loggingOptions),
       featureFlagsForFl(makeCompatJson(decompileCompatibilityFlagsForFl(api->getFeatureFlags()))),
       impl(kj::heap<Impl>(*api, *metrics, *limitEnforcer, inspectorPolicy)),
       weakIsolateRef(WeakIsolateRef::wrap(this)),
@@ -1512,10 +1505,8 @@ void setWebAssemblyModuleHasInstance(jsg::Lock& lock, v8::Local<v8::Context> con
       module->DefineOwnProperty(context, v8::Symbol::GetHasInstance(lock.v8Isolate), function));
 }
 
-void Worker::setupContext(jsg::Lock& lock,
-    v8::Local<v8::Context> context,
-    Worker::ConsoleMode consoleMode,
-    StructuredLogging structuredLogging) {
+void Worker::setupContext(
+    jsg::Lock& lock, v8::Local<v8::Context> context, const LoggingOptions& loggingOptions) {
   // Set WebAssembly.Module @@HasInstance
   setWebAssemblyModuleHasInstance(lock, context);
 
@@ -1531,9 +1522,9 @@ void Worker::setupContext(jsg::Lock& lock,
         lock.v8Isolate, jsg::check(console->Get(context, methodStr)).As<v8::Function>());
 
     auto f = lock.wrapSimpleFunction(context,
-        [consoleMode, level, structuredLogging, original = kj::mv(original)](
+        [loggingOptions, level, original = kj::mv(original)](
             jsg::Lock& js, const v8::FunctionCallbackInfo<v8::Value>& info) {
-      handleLog(js, consoleMode, level, structuredLogging, original, info);
+      handleLog(js, loggingOptions, level, original, info);
     });
     jsg::check(console->Set(context, methodStr, f));
   };
@@ -1848,9 +1839,8 @@ void Worker::processEntrypointClass(jsg::Lock& js,
 }
 
 void Worker::handleLog(jsg::Lock& js,
-    ConsoleMode consoleMode,
+    const LoggingOptions& loggingOptions,
     LogLevel level,
-    StructuredLogging structuredLogging,
     const v8::Global<v8::Function>& original,
     const v8::FunctionCallbackInfo<v8::Value>& info) {
   // Call original V8 implementation so messages sent to connected inspector if any
@@ -1947,7 +1937,7 @@ void Worker::handleLog(jsg::Lock& js,
     }
   }
 
-  if (consoleMode == ConsoleMode::INSPECTOR_ONLY) {
+  if (loggingOptions.consoleMode == LoggingOptions::ConsoleMode::INSPECTOR_ONLY) {
     // Lets us dump console.log()s to stdout when running test-runner with --verbose flag, to make
     // it easier to debug tests.  Note that when --verbose is not passed, KJ_LOG(INFO, ...) will
     // not even evaluate its arguments, so `message()` will not be called at all.
@@ -1966,7 +1956,7 @@ void Worker::handleLog(jsg::Lock& js,
 
     // Log warnings and errors to stderr
     // Always log to stdout when structuredLogging is enabled.
-    auto useStderr = level >= LogLevel::WARN && !structuredLogging;
+    auto useStderr = level >= LogLevel::WARN && !loggingOptions.structuredLogging;
     auto fd = useStderr ? stderr : stdout;
     auto tty = useStderr ? STDERR_TTY : STDOUT_TTY;
     auto colors =
@@ -1980,7 +1970,7 @@ void Worker::handleLog(jsg::Lock& js,
 
     auto levelStr = logLevelToString(level);
     args[length] = v8::Boolean::New(js.v8Isolate, colors);
-    args[length + 1] = v8::Boolean::New(js.v8Isolate, bool(structuredLogging));
+    args[length + 1] = v8::Boolean::New(js.v8Isolate, bool(loggingOptions.structuredLogging));
     args[length + 2] = jsg::v8StrIntern(js.v8Isolate, levelStr);
     auto formatted = js.toString(
         jsg::check(formatLog->Call(context, js.v8Undefined(), length + 3, args.data())));
@@ -3254,7 +3244,7 @@ void Worker::Isolate::logWarning(kj::StringPtr description, Lock& lock) {
     });
   }
 
-  if (consoleMode == ConsoleMode::INSPECTOR_ONLY) {
+  if (loggingOptions.consoleMode == LoggingOptions::ConsoleMode::INSPECTOR_ONLY) {
     // Run with --verbose to log JS exceptions to stderr. Useful when running tests.
     KJ_LOG(INFO, "console warning", description);
   } else {

src/workerd/io/worker.h

@@ -35,6 +35,19 @@ namespace workerd {
 WD_STRONG_BOOL(StructuredLogging);
 WD_STRONG_BOOL(ProcessStdioPrefixed);
 
+struct LoggingOptions {
+  enum class ConsoleMode {
+    // Only send `console.log`s to the inspector. Default, production behavior.
+    INSPECTOR_ONLY,
+    // Send `console.log`s to the inspector and stdout/err. Behavior running `workerd` locally.
+    STDOUT,
+  };
+
+  ConsoleMode consoleMode = ConsoleMode::INSPECTOR_ONLY;
+  StructuredLogging structuredLogging = StructuredLogging::NO;
+  ProcessStdioPrefixed processStdioPrefixed = ProcessStdioPrefixed::YES;
+};
+
 namespace api {
 class DurableObjectState;
 class DurableObjectStorage;
@@ -112,13 +125,6 @@ class Worker: public kj::AtomicRefcounted {
 
   class LockType;
 
-  enum class ConsoleMode {
-    // Only send `console.log`s to the inspector. Default, production behavior.
-    INSPECTOR_ONLY,
-    // Send `console.log`s to the inspector and stdout/err. Behavior running `workerd` locally.
-    STDOUT,
-  };
-
   explicit Worker(kj::Own<const Script> script,
       kj::Own<WorkerObserver> metrics,
       kj::FunctionParam<void(jsg::Lock& lock,
@@ -184,10 +190,8 @@ class Worker: public kj::AtomicRefcounted {
   void setConnectOverride(kj::String networkAddress, ConnectFn connectFn);
   kj::Maybe<ConnectFn&> getConnectOverride(kj::StringPtr networkAddress);
 
-  static void setupContext(jsg::Lock& lock,
-      v8::Local<v8::Context> context,
-      Worker::ConsoleMode consoleMode,
-      StructuredLogging structuredLogging);
+  static void setupContext(
+      jsg::Lock& lock, v8::Local<v8::Context> context, const LoggingOptions& loggingOptions);
 
  private:
   kj::Own<const Script> script;
@@ -213,9 +217,8 @@ class Worker: public kj::AtomicRefcounted {
   friend constexpr bool _kj_internal_isPolymorphic(AsyncWaiter*);
 
   static void handleLog(jsg::Lock& js,
-      ConsoleMode mode,
+      const LoggingOptions& loggingOptions,
       LogLevel level,
-      StructuredLogging structuredLogging,
       const v8::Global<v8::Function>& original,
       const v8::FunctionCallbackInfo<v8::Value>& info);
 
@@ -333,9 +336,7 @@ class Worker::Isolate: public kj::AtomicRefcounted {
       kj::StringPtr id,
       kj::Own<IsolateLimitEnforcer> limitEnforcer,
       InspectorPolicy inspectorPolicy,
-      ConsoleMode consoleMode = ConsoleMode::INSPECTOR_ONLY,
-      StructuredLogging structuredLogging = StructuredLogging::NO,
-      ProcessStdioPrefixed processStdioPrefixed = ProcessStdioPrefixed::YES);
+      LoggingOptions loggingOptions = {});
 
   ~Isolate() noexcept(false);
   KJ_DISALLOW_COPY_AND_MOVE(Isolate);
@@ -451,6 +452,11 @@ class Worker::Isolate: public kj::AtomicRefcounted {
 
   bool isInspectorEnabled() const;
 
+  // Get the process stdio prefixed setting from logging options
+  inline ProcessStdioPrefixed getProcessStdioPrefixed() const {
+    return loggingOptions.processStdioPrefixed;
+  }
+
   // Represents a weak reference back to the isolate that code within the isolate can use as an
   // indirect pointer when they want to be able to race destruction safely. A caller wishing to
   // use a weak reference to the isolate should acquire a strong reference to weakIsolateRef.
@@ -479,9 +485,7 @@ class Worker::Isolate: public kj::AtomicRefcounted {
   kj::String id;
   kj::Own<IsolateLimitEnforcer> limitEnforcer;
   kj::Own<Api> api;
-  ConsoleMode consoleMode;
-  StructuredLogging structuredLogging;
-  ProcessStdioPrefixed processStdioPrefixed;
+  LoggingOptions loggingOptions;
 
   // If non-null, a serialized JSON object with a single "flags" property, which is a list of
   // compatibility enable-flags that are relevant to FL.