Improved server robustness (#4)

* Improved server robustness

* Stop unlinking request / response json

* Add command-server.backgroundWindowProtection

* readme

Author: pokey

.eslintrc.json

@@ -12,8 +12,14 @@
         "@typescript-eslint/naming-convention": "warn",
         "@typescript-eslint/semi": "warn",
         "curly": "warn",
-        "eqeqeq": "warn",
+        "eqeqeq": [
+            "warn",
+            "always",
+            {
+                "null": "never"
+            }
+        ],
         "no-throw-literal": "warn",
         "semi": "off"
     }
-}
+}

.vscode/settings.json

@@ -7,5 +7,8 @@
         "out": true // set this to false to include "out" folder in search results
     },
     // Turn off tsc task auto detection since we have the necessary tasks as npm scripts
-    "typescript.tsc.autoDetect": "off"
+    "typescript.tsc.autoDetect": "off",
+    "cSpell.words": [
+        "eqeqeq"
+    ]
 }

README.md

@@ -55,6 +55,17 @@ Contributes the following commands:
 ## Configuration
 Contributes the following settings:
 
+### `command-server.backgroundWindowProtection`
+Turn this off if you're frequently seeing an error saying "This editor is not active".
+
+```json
+{
+    "command-server.backgroundWindowProtection": false
+}
+```
+
+Defaults to `true` (protection enabled).
+
 ### `command-server.allowList`
 Allows user to specify the allowed commands using glob syntax, eg:
 
@@ -82,12 +93,22 @@ Defaults to `[]` (doesn't deny anything).
 
 ## Known issues
 
+- If you see errors saying "This editor is not active", disable
+  `command-server.backgroundWindowProtection`, as described above.  VSCode
+  seems to be a bit inconsistent with determining which window has
+  focus.  There is code in the command server that tries to prevent a
+  background window from inadvertently executing a command, but when the
+  focused window detection fails, it will end up preventing correct commands
+  from running.  
 - The server won't respond until the extension is loaded.  This may be obvious,
   but just be aware that if you have other extensions that take a while to
   load, the server might not respond for a little while after you load an
   editor window until everything is fully loaded.
 
 ## Release Notes
 
+### 0.5.0
+- Improve robustness, and add `command-server.backgroundWindowProtection` setting
+
 ### 0.4.0
 - Switch to file-based RPC

package.json

@@ -8,7 +8,7 @@
 		"type": "git",
 		"url": "https://github.com/pokey/command-server"
 	},
-	"version": "0.4.0",
+	"version": "0.5.0",
 	"engines": {
 		"vscode": "^1.53.0"
 	},
@@ -50,6 +50,11 @@
 					"type": "array",
 					"default": [],
 					"description": "Commands to deny.  Supports simple glob syntax"
+				},
+				"command-server.backgroundWindowProtection": {
+					"type": "boolean",
+					"default": true,
+					"description": "Whether to enable protection against background windows executing a command"
 				}
 			}
 		}

src/commandRunner.ts

@@ -1,12 +1,16 @@
+import { open } from "fs/promises";
 import { Minimatch } from "minimatch";
 import * as vscode from "vscode";
 
 import { readRequest, writeResponse } from "./io";
+import { getResponsePath } from "./paths";
 import { any } from "./regex";
+import { Request, Response } from "./types";
 
 export default class CommandRunner {
   allowRegex!: RegExp;
   denyRegex!: RegExp | null;
+  backgroundWindowProtection!: boolean;
 
   constructor() {
     this.reloadConfiguration = this.reloadConfiguration.bind(this);
@@ -33,6 +37,10 @@ export default class CommandRunner {
       denyList.length === 0
         ? null
         : any(...denyList.map((glob) => new Minimatch(glob).makeRe()));
+
+    this.backgroundWindowProtection = vscode.workspace
+      .getConfiguration("command-server")
+      .get<boolean>("backgroundWindowProtection")!;
   }
 
   /**
@@ -43,12 +51,29 @@ export default class CommandRunner {
    * types.
    */
   async runCommand() {
+    const responseFile = await open(getResponsePath(), "wx");
+
+    var request: Request;
+
+    try {
+      request = await readRequest();
+    } catch (err) {
+      await responseFile.close();
+      throw err;
+    }
+
     const { commandId, args, uuid, returnCommandOutput, waitForFinish } =
-      await readRequest();
+      request;
+
+    const warnings = [];
 
     try {
       if (!vscode.window.state.focused) {
-        throw new Error("This editor is not active");
+        if (this.backgroundWindowProtection) {
+          throw new Error("This editor is not active");
+        } else {
+          warnings.push("This editor is not active");
+        }
       }
 
       if (!commandId.match(this.allowRegex)) {
@@ -69,16 +94,20 @@ export default class CommandRunner {
         await commandPromise;
       }
 
-      await writeResponse({
+      await writeResponse(responseFile, {
         error: null,
         uuid,
         returnValue: commandReturnValue,
+        warnings,
       });
     } catch (err) {
-      await writeResponse({
+      await writeResponse(responseFile, {
         error: err.message,
         uuid,
+        warnings,
       });
     }
+
+    await responseFile.close();
   }
 }

src/fileUtils.ts

@@ -1,26 +1,11 @@
-import { unlinkSync } from "fs";
-import { writeFile } from "fs/promises";
+import { FileHandle } from "fs/promises";
 
 /**
- * Opens a file exclusively, failing if it exists, and writes stringified JSON.
+ * Writes stringified JSON.
  * Appends newline so that other side knows when it is done
  * @param path Output path
  * @param body Body to stringify and write
  */
-export async function writeJSONExclusive(path: string, body: any) {
-  await writeFile(path, `${JSON.stringify(body)}\n`, { flag: "wx" });
-}
-
-/**
- * Unlink the given file if it exists, otherwise do nothing
- * @param path The path to unlink
- */
-export function unlinkIfExistsSync(path: string) {
-  try {
-    unlinkSync(path);
-  } catch (err) {
-    if (err.code !== "ENOENT") {
-      throw err;
-    }
-  }
+export async function writeJSON(file: FileHandle, body: any) {
+  await file.write(`${JSON.stringify(body)}\n`);
 }

src/initializeCommunicationDir.ts

@@ -1,11 +1,8 @@
 import { mkdirSync, lstatSync } from "fs";
 import { S_IWOTH } from "constants";
 import {
-  getRequestPath,
-  getResponsePath,
   getCommunicationDirPath,
 } from "./paths";
-import { unlinkIfExistsSync } from "./fileUtils";
 import { userInfo } from "os";
 
 export function initializeCommunicationDir() {
@@ -29,7 +26,4 @@ export function initializeCommunicationDir() {
       `Refusing to proceed because of invalid communication dir ${communicationDirPath}`
     );
   }
-
-  unlinkIfExistsSync(getRequestPath());
-  unlinkIfExistsSync(getResponsePath());
 }

src/io.ts

@@ -1,8 +1,8 @@
-import { readFile, stat, unlink } from "fs/promises";
+import { FileHandle, readFile, stat, unlink } from "fs/promises";
 import { STALE_TIMEOUT_MS, VSCODE_COMMAND_TIMEOUT_MS } from "./constants";
 import { getRequestPath, getResponsePath } from "./paths";
 import { Request, Response } from "./types";
-import { writeJSONExclusive } from "./fileUtils";
+import { writeJSON } from "./fileUtils";
 
 /**
  * Reads the JSON-encoded request from the request file, unlinking the file
@@ -15,8 +15,6 @@ export async function readRequest(): Promise<Request> {
   const stats = await stat(requestPath);
   const request = JSON.parse(await readFile(requestPath, "utf-8"));
 
-  await unlink(requestPath);
-
   if (
     Math.abs(stats.mtimeMs - new Date().getTime()) > VSCODE_COMMAND_TIMEOUT_MS
   ) {
@@ -29,38 +27,10 @@ export async function readRequest(): Promise<Request> {
 }
 
 /**
- * Writes the response to the response file as JSON.  If a stale response file
- * exists, it will be removed.  If a non-stale response file exists, assumes
- * there is another VSCode instance mysteriously trying to handle the request
- * as well and fails.
+ * Writes the response to the response file as JSON.
+ * @param file The file to write to
  * @param response The response object to JSON-encode and write to disk
  */
-export async function writeResponse(response: Response) {
-  const responsePath = getResponsePath();
-
-  try {
-    await writeJSONExclusive(responsePath, response);
-  } catch (err) {
-    if (err.code !== "EEXIST") {
-      throw err;
-    }
-
-    try {
-      const stats = await stat(responsePath);
-
-      if (Math.abs(stats.mtimeMs - new Date().getTime()) < STALE_TIMEOUT_MS) {
-        throw new Error("Another process has an active response file");
-      }
-
-      console.log("Removing stale response file");
-      await unlink(responsePath);
-    } catch (err) {
-      // If the file was removed for whatever reason in the interim we just continue
-      if (err.code !== "ENOENT") {
-        throw err;
-      }
-    }
-
-    await writeJSONExclusive(responsePath, response);
-  }
+export async function writeResponse(file: FileHandle, response: Response) {
+  await writeJSON(file, response);
 }

src/paths.ts

@@ -12,13 +12,9 @@ export function getCommunicationDirPath() {
 }
 
 export function getRequestPath() {
-  const communicationDirPath = getCommunicationDirPath();
-
-  return join(communicationDirPath, "request.json");
+  return join(getCommunicationDirPath(), "request.json");
 }
 
 export function getResponsePath() {
-  const communicationDirPath = getCommunicationDirPath();
-
-  return join(communicationDirPath, "response.json");
+  return join(getCommunicationDirPath(), "response.json");
 }

src/types.ts

@@ -45,4 +45,9 @@ export interface Response {
    * Any error encountered or null if successful
    */
   error: string | null;
+
+  /**
+   * A list of warnings issued when running the command
+   */
+  warnings: string[];
 }