Fix race condition when launching Oxide helper process (#1520)

Fixes #1519

We were calling `startIfNeeded()` but it did async work before assigning
the connection which means if scan() is called once per project the flow
looks like this:
Project 1-N: `scan()` -> `startIfNeeded()` -> check for existing
connection -> `await … ` -> setup connection -> done
  
Since projects are loaded via `Promise.all(…)` every single one will run
all the way up to the `await` call, wait, fork a process, setup an RPC
connection, and overwrite the `this.helper` and `this.connection`
properties. I think the additional processes would stick around because
of closure scopes but not 100% sure about that as they should've
otherwise been unreferenced.

This PR changes the setup in a few ways:
- The code that looks for the Oxide helper async is gone. The relative
path is resolved at startup.
- We synchronously store a promise to the object containing details to
access the helper process
- Most of the process is now synchronous with a small promise that waits
for the process to become ready.

Author: thecrypticace

esbuild.mjs

@@ -21,6 +21,9 @@ let ctx = await esbuild.context({
   platform: 'node',
   external: ['pnpapi', 'vscode', 'lightningcss', '@tailwindcss/oxide'],
   format: 'cjs',
+  define: {
+    'process.env.TEST': '0',
+  },
   outdir: args.outdir,
   outfile: args.outfile,
   minify: args.minify,

packages/tailwindcss-language-server/src/oxide-helper.ts

@@ -10,5 +10,11 @@ let connection = rpc.createMessageConnection(
 
 let scanRequest = new rpc.RequestType<ScanOptions, ScanResult, void>('scan')
 connection.onRequest<ScanOptions, ScanResult, void>(scanRequest, (options) => scan(options))
-
 connection.listen()
+
+console.log('Listening for messages...')
+
+process.on('disconnect', () => {
+  console.log('Shutting down...')
+  process.exit(0)
+})

packages/tailwindcss-language-server/src/oxide-session.ts

@@ -1,9 +1,29 @@
 import * as rpc from 'vscode-jsonrpc/node'
 import * as proc from 'node:child_process'
 import * as path from 'node:path'
-import * as fs from 'node:fs/promises'
 import { type ScanOptions, type ScanResult } from './oxide'
 
+interface ServerHandle {
+  helper: proc.ChildProcess
+  connection: rpc.MessageConnection
+}
+
+/**
+ * The path to the Oxide helper process
+ *
+ * TODO:
+ * - Can we find a way to not require a build first — i.e. point to
+ *   `oxide-helper.ts` and have things "hot reload" during tests?
+ */
+const helperPath = process.env.TEST
+  ? // This first path is relative to the source file so running tests in Vitest
+    // result in the correct path — does still point to the built files.
+    path.resolve(path.dirname(__filename), '../bin/oxide-helper.js')
+  : // The second path is relative to the built file. This is the same for the
+    // language server *and* the extension since the file is named identically
+    // in both builds.
+    path.resolve(path.dirname(__filename), './oxide-helper.js')
+
 /**
  * This helper starts a session in which we can use Oxide in *another process*
  * to communicate content scanning results.
@@ -19,75 +39,64 @@ import { type ScanOptions, type ScanResult } from './oxide'
  *   us sidestep the problem as the process will only be running as needed.
  */
 export class OxideSession {
-  helper: proc.ChildProcess | null = null
-  connection: rpc.MessageConnection | null = null
+  /**
+   * An object that represents the connection to the server
+   *
+   * This ensures that either everything is initialized or nothing is
+   */
+  private server: Promise<ServerHandle> | null = null
 
   public async scan(options: ScanOptions): Promise<ScanResult> {
-    await this.startIfNeeded()
+    let server = await this.startIfNeeded()
 
-    return await this.connection.sendRequest('scan', options)
+    return await server.connection.sendRequest('scan', options)
   }
 
-  async startIfNeeded(): Promise<void> {
-    if (this.connection) return
-
-    // TODO: Can we find a way to not require a build first?
-    // let module = path.resolve(path.dirname(__filename), './oxide-helper.ts')
-
-    let modulePaths = [
-      // Separate Language Server package
-      '../bin/oxide-helper.js',
-
-      // Bundled with the VSCode extension
-      '../dist/oxide-helper.js',
-    ]
+  startIfNeeded(): Promise<ServerHandle> {
+    this.server ??= this.start()
 
-    let module: string | null = null
-
-    for (let relativePath of modulePaths) {
-      let filepath = path.resolve(path.dirname(__filename), relativePath)
-
-      if (
-        await fs.access(filepath).then(
-          () => true,
-          () => false,
-        )
-      ) {
-        module = filepath
-        break
-      }
-    }
+    return this.server
+  }
 
-    if (!module) throw new Error('unable to load')
+  private async start(): Promise<ServerHandle> {
+    // 1. Start the new process
+    let helper = proc.fork(helperPath)
+
+    // 2. If the process fails to spawn we want to throw
+    //
+    // We do end up caching the failed promise but that should be
+    // fine. It seems unlikely that, if this fails, trying again
+    // would "fix" whatever problem there was and succeed.
+    await new Promise((resolve, reject) => {
+      helper.on('spawn', resolve)
+      helper.on('error', reject)
+    })
 
-    let helper = proc.fork(module)
+    // 3. Setup a channel to talk to the server
     let connection = rpc.createMessageConnection(
       new rpc.IPCMessageReader(helper),
       new rpc.IPCMessageWriter(helper),
     )
 
-    helper.on('disconnect', () => {
+    // 4. If the process exits we can tear down everything
+    helper.on('close', () => {
       connection.dispose()
-      this.connection = null
-      this.helper = null
-    })
-
-    helper.on('exit', () => {
-      connection.dispose()
-      this.connection = null
-      this.helper = null
+      this.server = null
     })
 
+    // 5. Start listening for messages
     connection.listen()
 
-    this.helper = helper
-    this.connection = connection
+    return { helper, connection }
   }
 
   async stop() {
-    if (!this.helper) return
+    if (!this.server) return
+
+    let server = await this.server
 
-    this.helper.disconnect()
-    this.helper.kill()
+    // We terminate the server because, if for some reason it gets stuck,
+    // we don't want it to stick around.
+    server.helper.kill()
   }
 }

packages/tailwindcss-language-server/vitest.config.mts

@@ -8,6 +8,10 @@ export default defineConfig({
     silent: 'passed-only',
   },
 
+  define: {
+    'process.env.TEST': '1',
+  },
+
   plugins: [
     tsconfigPaths(),
     {

packages/tailwindcss-language-service/vitest.config.mts

@@ -5,4 +5,8 @@ export default defineConfig({
     testTimeout: 15000,
     silent: 'passed-only',
   },
+
+  define: {
+    'process.env.TEST': '1',
+  },
 })

packages/tailwindcss-language-syntax/vitest.config.mts

@@ -5,4 +5,8 @@ export default defineConfig({
     testTimeout: 15000,
     silent: 'passed-only',
   },
+
+  define: {
+    'process.env.TEST': '1',
+  },
 })