Repository: stateful/vscode-runme
Pull request #358 (2023-03-29 16:02:28)

Fix relaunch behavior in runme server logic + window-specific certificates
This PR does the following:

 - Fix relaunch behavior on close, which could sometimes be called in tandem with the relaunch behavior on error, causing two concurrent relaunches
 - Create TLS certificates in a different directory per-window rather than globally 
   - This behavior is overridden when a specific TLS directory is set - so the issue will still exist when the user specificies a directory, because runme CLI will always generate a new certificate on launch rather than reading an existing one)
 - Clean up some logic in the runme server to be iterative rather than recursive, which helps for debugging & code clarity


Patch:
diff --git a/package-lock.json b/package-lock.json
index 02a5d9db..b9419d5f 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,12 +1,12 @@
 {
   "name": "runme",
-  "version": "1.0.1",
+  "version": "1.0.2-edge.0",
   "lockfileVersion": 2,
   "requires": true,
   "packages": {
     "": {
       "name": "runme",
-      "version": "1.0.1",
+      "version": "1.0.2-edge.0",
       "hasInstallScript": true,
       "license": "Apache-2.0",
       "dependencies": {
diff --git a/package.json b/package.json
index c9660382..51af9338 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "runme",
-  "version": "1.0.1",
+  "version": "1.0.2-edge.0",
   "publisher": "stateful",
   "displayName": "Runme",
   "description": "Run your README.md as an interactive notebook",
diff --git a/src/extension/extension.ts b/src/extension/extension.ts
index 580e2fb6..64f90087 100644
--- a/src/extension/extension.ts
+++ b/src/extension/extension.ts
@@ -34,7 +34,7 @@ export class RunmeExtension {
     const grpcRunner = kernel.hasExperimentEnabled('grpcRunner')
     const server = new RunmeServer(context.extensionUri, {
       retryOnFailure: true,
-      maxNumberOfIntents: 2,
+      maxNumberOfIntents: 10,
     }, !grpcServer, grpcRunner)
 
     let runner: IRunner|undefined
diff --git a/src/extension/server/runmeServer.ts b/src/extension/server/runmeServer.ts
index 47bf087f..d92b06db 100644
--- a/src/extension/server/runmeServer.ts
+++ b/src/extension/server/runmeServer.ts
@@ -31,11 +31,11 @@ class RunmeServer implements Disposable {
     #retryOnFailure: boolean
     #maxNumberOfIntents: number
     #loggingEnabled: boolean
-    #intent: number
     #acceptsIntents: number
     #acceptsInterval: number
     #disposables: Disposable[] = []
     #transport?: GrpcTransport
+    #serverDisposables: Disposable[] = []
 
     readonly #onClose = this.register(new EventEmitter<{ code: number|null }>())
     readonly #onTransportReady = this.register(new EventEmitter<{ transport: GrpcTransport }>())
@@ -54,7 +54,6 @@ class RunmeServer implements Disposable {
       this.#binaryPath = getBinaryPath(extBasePath, process.platform)
       this.#retryOnFailure = options.retryOnFailure || false
       this.#maxNumberOfIntents = options.maxNumberOfIntents
-      this.#intent = 0
       this.#acceptsIntents = options.acceptsConnection?.intents || 50
       this.#acceptsInterval = options.acceptsConnection?.interval || 200
     }
@@ -69,6 +68,7 @@ class RunmeServer implements Disposable {
 
       if(process === this.#process) {
         this.#process = undefined
+        this.clearServerDisposables()
       }
 
       process?.removeAllListeners()
@@ -176,9 +176,6 @@ class RunmeServer implements Disposable {
             this.#onClose.fire({ code })
 
             this.disposeProcess(process)
-
-            // try to relaunch
-            this.launch()
         })
 
 
@@ -198,20 +195,39 @@ class RunmeServer implements Disposable {
           [
             new Promise<string>((resolve, reject) => {
               const cb = (data: any) => {
-                const msg = data.toString()
+                const msg: string = data.toString()
                 try {
-                  const log = JSON.parse(msg)
-                  if (log.addr) {
-                    process.stderr.off('data', cb)
-                    return resolve(log.addr)
+                  for (const line of msg.split('\n')) {
+                    if (!line) {
+                      continue
+                    }
+
+                    let log: any
+
+                    try {
+                      log = JSON.parse(line)
+                    } catch(e) {
+                      continue
+                    }
+
+                    if (log.addr) {
+                      process.stderr.off('data', cb)
+                      return resolve(log.addr)
+                    }
                   }
                 } catch (err: any) {
-                    reject(new RunmeServerError(`Server failed, reason: ${msg || (err as Error).message}`))
+                    reject(new RunmeServerError(`Server failed, reason: ${(err as Error).message}`))
                 }
               }
 
               process.stderr.on('data', cb)
             }),
+            new Promise<never>((_, reject) => {
+              const { dispose } = this.#onClose.event(() => {
+                dispose()
+                reject(new Error('Server closed prematurely!'))
+              })
+            }),
             new Promise<never>((_, reject) => setTimeout(
               () => reject(new Error('Timed out listening for server ready message')),
               10000
@@ -223,28 +239,21 @@ class RunmeServer implements Disposable {
     async acceptsConnection(): Promise<void> {
         const INTERVAL = this.#acceptsInterval
         const INTENTS = this.#acceptsIntents
-        let token: NodeJS.Timer
         let iter = 0
         let isRunning = false
-        const ping = (resolve: Function, reject: Function) => {
-          return async () => {
-              iter++
-              isRunning = await this.isRunning()
-              if (isRunning) {
-                  clearTimeout(token)
-                  return resolve()
-              } else if (iter > INTENTS) {
-                  clearTimeout(token)
-                  return reject(new RunmeServerError(`Server did not accept connections after ${iter*INTERVAL}ms`))
-              }
-              if (!token) {
-                  token = setInterval(ping(resolve, reject), INTERVAL)
-              }
+
+        while (iter < INTENTS) {
+          isRunning = await this.isRunning()
+          if (isRunning) {
+            return
           }
+
+          await new Promise(r => setInterval(r, INTERVAL))
+
+          iter++
         }
-        return new Promise<void>((resolve, reject) => {
-            return ping(resolve, reject)()
-        })
+
+        throw new RunmeServerError(`Server did not accept connections after ${iter*INTERVAL}ms`)
     }
 
     /**
@@ -255,7 +264,9 @@ class RunmeServer implements Disposable {
      *
      * @returns Address of server or error
      */
-    async launch(): Promise<string> {
+    async launch(intent = 0): Promise<string> {
+      this.disposeProcess()
+
       if (this.externalServer) {
         await this.connect()
         return this.address()
@@ -265,16 +276,23 @@ class RunmeServer implements Disposable {
       try {
           addr = await this.start()
       } catch (e) {
-        if (this.#retryOnFailure && this.#maxNumberOfIntents > this.#intent) {
+        if (this.#retryOnFailure && this.#maxNumberOfIntents > intent) {
             console.error(`Failed to start runme server, retrying. Error: ${(e as Error).message}`)
-            this.#intent++
-            return this.launch()
+            return this.launch(intent + 1)
           }
           throw new RunmeServerError(`Cannot start server. Error: ${(e as Error).message}`)
       }
 
       await this.connect()
 
+      // relaunch on close
+      this.registerServerDisposable(
+        this.#onClose.event(() => {
+          this.launch()
+          this.#serverDisposables.forEach(({ dispose }) => dispose())
+        })
+      )
+
       return addr
     }
 
@@ -293,6 +311,15 @@ class RunmeServer implements Disposable {
       this.#disposables.push(disposable)
       return disposable
     }
+
+    private registerServerDisposable<T extends Disposable>(d: T) {
+      this.#serverDisposables.push(d)
+    }
+
+    private clearServerDisposables() {
+      this.#serverDisposables.forEach(({ dispose }) => dispose())
+      this.#serverDisposables = []
+    }
 }
 
 export default RunmeServer
diff --git a/src/utils/configuration.ts b/src/utils/configuration.ts
index a1760c45..f0b96f97 100644
--- a/src/utils/configuration.ts
+++ b/src/utils/configuration.ts
@@ -3,13 +3,14 @@ import os from 'node:os'
 
 import { NotebookCell, Uri, workspace } from 'vscode'
 import { z } from 'zod'
+import { v4 as uuidv4 } from 'uuid'
 
 import { isWindows } from '../extension/executors/utils'
 import { getAnnotations } from '../extension/utils'
 
 const SERVER_SECTION_NAME = 'runme.server'
 const TERMINAL_SECTION_NAME= 'runme.terminal'
-export const DEFAULT_TLS_DIR = path.join(os.tmpdir(), 'runme', 'tls')
+export const DEFAULT_TLS_DIR = path.join(os.tmpdir(), 'runme', uuidv4(), 'tls')
 
 type NotebookTerminalValue = keyof typeof configurationSchema.notebookTerminal
 


Test patch:
diff --git a/tests/extension/server/runmeServer.test.ts b/tests/extension/server/runmeServer.test.ts
index 564d77fe..1a6013b7 100644
--- a/tests/extension/server/runmeServer.test.ts
+++ b/tests/extension/server/runmeServer.test.ts
@@ -155,7 +155,29 @@ suite('Runme server accept connections', () => {
         await expect(
           server.launch()
         ).rejects.toThrowErrorMatchingInlineSnapshot(
-          '"Server did not accept connections after 5ms"'
+          '"Server did not accept connections after 4ms"'
         )
     })
 })
+
+test('Runme server clears server disposables', () => {
+  const server = new Server(
+    Uri.file('/Users/user/.vscode/extension/stateful.runme'),
+    {
+      retryOnFailure: false,
+      maxNumberOfIntents: 2,
+      acceptsConnection: {
+        intents: 4,
+        interval: 1,
+      }
+    },
+    false
+  )
+
+  const dispose = vi.fn()
+
+  server['registerServerDisposable']({ dispose })
+  server['clearServerDisposables']()
+
+  expect(dispose).toHaveBeenCalledTimes(1)
+})