Merge pull request #8 from AtomLinter/node-path-tester-tests

Add NodePathTester tests

Author: savetheclocktower

lib/config.js

@@ -91,7 +91,7 @@ const Config = {
     if (!configChanged(newConfig, this._currentConfig)) { return; }
     if (this._currentConfig) {
       for (let handler of this.handlers) {
-        handler(newConfig, this._currentConfig || {});
+        handler(newConfig, this._currentConfig);
       }
     }
     this._currentConfig = newConfig;

lib/job-manager.js

@@ -28,63 +28,97 @@ class InvalidWorkerError extends Error {
   }
 }
 
+// A class that handles creating, maintaining, and communicating with the
+// worker that we spawn to perform linting.
 class JobManager {
   constructor () {
     this.handlersForJobs = new Map();
     this.worker = null;
     this.workerPath = Path.join(__dirname, 'worker.js');
-
-    this.createWorker();
   }
 
   dispose () {
     this.killWorker();
+    this.worker = null;
+    this._workerPromise = null;
+    this.handlersForJobs = null;
   }
 
+  // Resolves when the worker is spawned and ready to process messages. Rejects
+  // if the worker errors during startup.
   createWorker () {
+    // When reloading an existing project with X tabs open, this method will be
+    // called X times in a very short span of time. They all need to wait for
+    // the _same_ worker to spawn instead of each trying to spawn their own.
+    if (this._workerPromise) {
+      // The worker is already in the process of being created.
+      return this._workerPromise;
+    }
+
     let nodeBin = Config.get('nodeBin');
-    console.debug('JobManager creating worker at:', nodeBin, this.workerPath);
+    console.debug('JobManager creating worker at:', nodeBin);
     this.killWorker();
 
-    // We should not try to start the worker without testing the value we have
-    // for `nodeBin`.
+    // We choose to do a sync test here because this method is much easier to
+    // reason about without an `await` keyword introducing side effects.
     //
-    // When the setting is changed after initialization, we test it
-    // asynchronously before calling `createWorker` again. In those cases,
-    // `testSync` just looks up the result of that test so we don't duplicate
-    // effort.
+    // In practice, this will result in only one brief call to `execSync` when
+    // a project window is created/reloaded; subsequent calls with the same
+    // `nodeBin` argument will reuse the earlier value.
     //
-    // But on startup, we don't want to defer creation of this worker while we
-    // perform an async test of `nodeBin`. So in that one scenario, `testSync`
-    // will do an `execSync` on this value to perform a sanity check. Like the
-    // async version, we remember this result, so further calls to `testSync`
-    // with the same value won't block while we run a shell command.
-    //
-    // TODO: See if there's a way to use the async test logic on startup
-    // without putting us in async/promise hell.
-    if (!NodePathTester.testSync(nodeBin)) {
-      console.error('Invalid nodeBin!');
+    // When `nodeBin` is changed in the middle of a session, we validate the
+    // new value asynchronously _before_ we reach this method, and `testSync`
+    // merely looks up the async validation's result.
+    let isValid = NodePathTester.testSync(nodeBin);
+    if (!isValid) {
       this.worker = false;
-      return false;
+      throw new InvalidWorkerError();
     }
 
-    this.worker = spawn(nodeBin, [this.workerPath]);
+    let promise = new Promise((resolve, reject) => {
+      this.worker = spawn(nodeBin, [this.workerPath]);
+
+      // Reject this promise if the worker fails to spawn.
+      this.worker.on('error', reject);
+
+      this.worker.stdout
+        .pipe(ndjson.parse())
+        .on('data', (obj) => {
+          // We could listen for the `spawn` event to know when the worker is
+          // ready, but that event wasn't added until Node v14.17. Instead,
+          // we'll just have the worker emit a `ready` message.
+          if (obj.type === 'ready') {
+            resolve();
+          } else {
+            this.receiveMessage(obj);
+          }
+        });
+
+      // Even unanticipated runtime errors will get sent as newline-delimited
+      // JSON.
+      this.worker.stderr
+        .pipe(ndjson.parse())
+        .on('data', this.receiveError.bind(this));
+
+      this.worker.on('close', () => {
+        if (this.worker.killed === false) {
+          this.createWorker();
+        }
+      });
+    });
 
-    this.worker.stdout
-      .pipe(ndjson.parse())
-      .on('data', this.receiveMessage.bind(this));
+    this._workerPromise = promise;
+    this._workerPromise
+      .then(() => this._workerPromise = null)
+      .catch(() => this._workerPromise = null);
 
-    // Even unanticipated runtime errors will get sent as newline-delimited
-    // JSON.
-    this.worker.stderr
-      .pipe(ndjson.parse())
-      .on('data', this.receiveError.bind(this));
+    return promise;
+  }
 
-    this.worker.on('close', () => {
-      if (this.worker.killed === false) {
-        this.createWorker();
-      }
-    });
+  suspend () {
+    console.warn('Suspending worker');
+    this.killWorker();
+    this.worker = null;
   }
 
   killWorker () {
@@ -136,20 +170,14 @@ class JobManager {
   }
 
   async send (bundle) {
+    if (!this.worker) {
+      console.warn('Creating worker');
+      await this.createWorker();
+    }
+
     let key = generateKey();
     bundle.key = key;
     console.debug('JobManager#send:', bundle);
-    try {
-      this.ensureWorker();
-    } catch (err) {
-      if (this.worker === false) {
-        // `false` means we intentionally refused to create a worker because
-        // `nodeBin` was invalid.
-        throw new InvalidWorkerError();
-      } else {
-        throw err;
-      }
-    }
 
     return new Promise((resolve, reject) => {
       this.handlersForJobs.set(key, [resolve, reject]);