fix: don't prevent worker exit on unhandledException (#37)

* fix: don't prevent worker exit on `unhandledException`

Fixes #16.

* feat: configurable debug logging

Can be enabled for extra information when submitting bug reports.

* chore: update URL

atom.io URLs all redirect to the sunset message, so this might as well link to
the Pulsar package repo.

* chore: update README to install from GitHub

…until we can publish to the Pulsar Package Repository.

* fix: ensure worker is killed when disposing JobManager

README.md

@@ -5,7 +5,7 @@ A “bring-your-own-Node” linter for newer versions of ESLint: v8 and above.
 ## Installation
 
 ```ShellSession
-apm install linter-eslint-node
+ppm install AtomLinter/linter-eslint-node
 ```
 
 The `linter` package will be installed for you if it’s not already present in your Atom installation. If you’re using an alternative `linter-*` consumer, the `linter` package can be disabled.

lib/console.js

@@ -2,7 +2,16 @@
 
 const CONSOLE = {};
 
-const isEnabled = atom.inDevMode() && !process.env.SILENCE_LOG;
+function getEnabled () {
+  if (process.env.SILENCE_LOG) { return false; }
+  return atom.config.get('linter-eslint-node.advanced.enableLogging');
+}
+
+let isEnabled = getEnabled();
+
+atom.config.observe('linter-eslint-node.advanced.enableLogging', () => {
+  isEnabled = getEnabled();
+});
 
 function makeConsoleMethod (name) {
   return (...args) => {

lib/job-manager.js

@@ -9,6 +9,9 @@ import console from './console';
 import Config from './config';
 import ndjson from 'ndjson';
 
+// How log we wait for a job to finish before giving up.
+const JOB_DURATION_TIMEOUT_MS = 15000;
+
 function generateKey () {
   return randomBytes(5).toString('hex');
 }
@@ -28,6 +31,29 @@ class InvalidWorkerError extends Error {
   }
 }
 
+class TimeoutError extends Error {
+  constructor (duration) {
+    super(`Operation timed out after ${duration}ms`);
+    this.name = 'TimeoutError';
+    this.duration = duration;
+  }
+}
+
+function rejectAfterDuration (duration) {
+  return new Promise((resolve, reject) => {
+    setTimeout(() => {
+      reject(new TimeoutError(duration));
+    }, duration);
+  });
+}
+
+function timeout (promise) {
+  return Promise.race([
+    rejectAfterDuration(JOB_DURATION_TIMEOUT_MS),
+    promise
+  ]);
+}
+
 // A class that handles creating, maintaining, and communicating with the
 // worker that we spawn to perform linting.
 class JobManager {
@@ -83,7 +109,19 @@ class JobManager {
       this.worker = spawn(nodeBin, [this.workerPath]);
 
       // Reject this promise if the worker fails to spawn.
-      this.worker.on('error', reject);
+      this.worker.on('error', (...args) => {
+        console.warn('Worker exited with error:', ...args);
+        reject(args);
+      });
+
+      this.worker.on('exit', ({ code }) => {
+        if (code !== 0) {
+          // The worker exited unexpectedly. We don't need to restart it now;
+          // we can wait until there's another task for the worker, and we
+          // don't want to cause a restart loop.
+          console.warn('Worker exited with code:', code);
+        }
+      });
 
       this.worker.stdout
         .pipe(ndjson.parse({ strict: false }))
@@ -130,6 +168,7 @@ class JobManager {
   }
 
   killWorker (worker) {
+    worker ??= this.worker;
     if (!worker || worker.exitCode) { return; }
     worker.removeAllListeners();
     worker.kill();
@@ -191,11 +230,13 @@ class JobManager {
     bundle.key = key;
     console.debug('JobManager#send:', bundle);
 
-    return new Promise((resolve, reject) => {
-      this.handlersForJobs.set(key, [resolve, reject]);
-      let str = JSON.stringify(bundle);
-      this.worker.stdin.write(`${str}\n`);
-    });
+    return timeout(
+      new Promise((resolve, reject) => {
+        this.handlersForJobs.set(key, [resolve, reject]);
+        let str = JSON.stringify(bundle);
+        this.worker.stdin.write(`${str}\n`);
+      })
+    );
   }
 }
 

lib/main.js

@@ -241,9 +241,26 @@ export default {
     this.notified.invalidNodeBin = true;
   },
 
+  notifyAboutTimeout (timeoutError, editor = null) {
+    let forASpecificEditor = '';
+    if (editor && editor.getPath) {
+      forASpecificEditor = `for \`${editor.getPath()}\``;
+    }
+    atom.notifications.addError(
+      `linter-eslint-node: Job timed out`, {
+        description: `A linting job ${forASpecificEditor}timed out after ${timeoutError.duration} milliseconds.`,
+        dismissable: true
+      }
+    );
+  },
+
   clearESLintCache () {
     console.debug('Telling the worker to clear its cache');
-    this.jobManager.send({ type: 'clear-cache' });
+    try {
+      this.jobManager.send({ type: 'clear-cache' });
+    } catch (err) {
+      this.handleError(err, 'clear-cache');
+    }
   },
 
   // Show a bunch of stuff to the user that can help them figure out why the
@@ -435,6 +452,13 @@ export default {
   },
 
   handleError (err, type, editor = null) {
+    if (err.name && err.name === 'TimeoutError') {
+      // An operation timed out. We should tell the user but should not sleep
+      // the worker, since we have no idea what caused the timeout or if it's
+      // likely to recur.
+      this.notifyAboutTimeout(err, editor && editor.getPath());
+      return null;
+    }
     if (err.name && err.name === 'InvalidWorkerError') {
       // The worker script never got created. Aside from notifying the user
       // (which will be skipped if they've already gotten such a message in
@@ -519,7 +543,7 @@ export default {
               {
                 text: 'Install linter-eslint',
                 onDidClick () {
-                  shell.openExternal(`https://atom.io/packages/linter-eslint`);
+                  shell.openExternal(`https://web.pulsar-edit.dev/packages/linter-eslint`);
                 }
               }
             ]

lib/worker.js

@@ -409,13 +409,27 @@ async function processMessage (bundle) {
 if (require.main === module) {
   process.stdin.pipe(ndjson.parse({ strict: false })).on('data', processMessage);
   process.stdin.resume();
+
   process.on('uncaughtException', (error) => {
     error.uncaught = true;
     error.error = 'Unknown error';
-    emitError(
-      JSON.stringify(error, Object.getOwnPropertyNames(error))
-    );
+
+    // We're catching this exception so that we can try to emit it as JSON
+    // before exiting. That way `JobManager` can at least attempt to report
+    // something meaningful to the user. But emitting the error as JSON might
+    // _itself_ fail, so we've got to guard against that, lest we start an
+    // infinite loop.
+    try {
+      emitError(
+        JSON.stringify(error, Object.getOwnPropertyNames(error))
+      );
+    } finally {
+      process.exit(1);
+    }
   });
+
   process.title = `node (linter-eslint-node worker ${process.pid})`;
+
+  // Signal to the package that we're ready to lint.
   emit({ type: 'ready' });
 }

package.json

@@ -145,6 +145,13 @@
           "type": "boolean",
           "default": true,
           "order": 3
+        },
+        "enableLogging": {
+          "title": "Enable Logging",
+          "description": "Log diagnostic messages to the developer console. If you want to file a bug against `linter-eslint-node`, these messages may be useful.",
+          "type": "boolean",
+          "default": false,
+          "order": 4
         }
       }
     }

spec/linter-eslint-node-spec.js

@@ -667,4 +667,20 @@ describe('The eslint provider for Linter', () => {
     });
   });
 
+  describe("When JobManager#killWorker is called", () => {
+    it('kills the worker', async () => {
+      let { jobManager } = linterEslintNode;
+      expect(!!jobManager).toBe(true);
+
+      let editor = await atom.workspace.open(paths.bad);
+      atom.project.setPaths([projectDir]);
+
+      // Kick off a job, then immediately kill the worker.
+      lint(editor);
+      expect(!!jobManager.worker).toBe(true);
+      jobManager.killWorker();
+      expect(jobManager.worker.killed).toBe(true);
+    });
+  });
+
 });