Repository: stateful/vscode-runme
Pull request #259 (2023-03-02 18:40:54)

One session per window
Runs one session per window, with incremental port assigning.

Note that there will be a race condition here (between deciding the port and running the server, the port can become occupied). The only way to avoid this would be putting the port assignment code on go, or by adding resilience to binding errors in the server process launch logic.

mxsdev: This is working by the way, just currently wrangling with tests
sourishkrout: > Note that there will be a race condition here (between deciding the port and running the server, the port can become occupied). The only way to avoid this would be putting the port assignment code on go, or by adding resilience to binding errors in the server process launch logic.

I think the risk is acceptably low since "the race" is between the same user's interactions. If multiple windows open all at once, the retry loop should make this work eventually.

Patch:
diff --git a/package-lock.json b/package-lock.json
index 228b398c..c1a9f53e 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -19,6 +19,7 @@
         "@vercel/frameworks": "^1.2.4",
         "@vscode/vscode-languagedetection": "^1.0.22",
         "@vscode/webview-ui-toolkit": "^1.2.1",
+        "detect-port": "^1.5.1",
         "filenamify": "^5.1.1",
         "get-port": "^6.1.2",
         "got": "^12.5.3",
@@ -6513,6 +6514,14 @@
         "node": ">=0.4.0"
       }
     },
+    "node_modules/address": {
+      "version": "1.2.2",
+      "resolved": "https://registry.npmjs.org/address/-/address-1.2.2.tgz",
+      "integrity": "sha512-4B/qKCfeE/ODUaAUpSwfzazo5x29WD4r3vXiWsB7I2mSDAihwEqKO+g8GELZUQSSAo5e1XTYh3ZVfLyxBc12nA==",
+      "engines": {
+        "node": ">= 10.0.0"
+      }
+    },
     "node_modules/agent-base": {
       "version": "6.0.2",
       "resolved": "https://registry.npmjs.org/agent-base/-/agent-base-6.0.2.tgz",
@@ -9136,6 +9145,19 @@
         "node": ">=8"
       }
     },
+    "node_modules/detect-port": {
+      "version": "1.5.1",
+      "resolved": "https://registry.npmjs.org/detect-port/-/detect-port-1.5.1.tgz",
+      "integrity": "sha512-aBzdj76lueB6uUst5iAs7+0H/oOjqI5D16XUWxlWMIMROhcM0rfsNVk93zTngq1dDNpoXRr++Sus7ETAExppAQ==",
+      "dependencies": {
+        "address": "^1.0.1",
+        "debug": "4"
+      },
+      "bin": {
+        "detect": "bin/detect-port.js",
+        "detect-port": "bin/detect-port.js"
+      }
+    },
     "node_modules/devtools": {
       "version": "8.3.11",
       "resolved": "https://registry.npmjs.org/devtools/-/devtools-8.3.11.tgz",
@@ -27750,6 +27772,11 @@
       "resolved": "https://registry.npmjs.org/acorn-walk/-/acorn-walk-8.2.0.tgz",
       "integrity": "sha512-k+iyHEuPgSw6SbuDpGQM+06HQUa04DZ3o+F6CSzXMvvI5KMvnaEqXe+YVe555R9nn6GPt404fos4wcgpw12SDA=="
     },
+    "address": {
+      "version": "1.2.2",
+      "resolved": "https://registry.npmjs.org/address/-/address-1.2.2.tgz",
+      "integrity": "sha512-4B/qKCfeE/ODUaAUpSwfzazo5x29WD4r3vXiWsB7I2mSDAihwEqKO+g8GELZUQSSAo5e1XTYh3ZVfLyxBc12nA=="
+    },
     "agent-base": {
       "version": "6.0.2",
       "resolved": "https://registry.npmjs.org/agent-base/-/agent-base-6.0.2.tgz",
@@ -29712,6 +29739,15 @@
       "resolved": "https://registry.npmjs.org/detect-newline/-/detect-newline-3.1.0.tgz",
       "integrity": "sha512-TLz+x/vEXm/Y7P7wn1EJFNLxYpUD4TgMosxY6fAVJUnJMbupHBOncxyWUG9OpTaH9EBD7uFI5LfEgmMOc54DsA=="
     },
+    "detect-port": {
+      "version": "1.5.1",
+      "resolved": "https://registry.npmjs.org/detect-port/-/detect-port-1.5.1.tgz",
+      "integrity": "sha512-aBzdj76lueB6uUst5iAs7+0H/oOjqI5D16XUWxlWMIMROhcM0rfsNVk93zTngq1dDNpoXRr++Sus7ETAExppAQ==",
+      "requires": {
+        "address": "^1.0.1",
+        "debug": "4"
+      }
+    },
     "devtools": {
       "version": "8.3.11",
       "resolved": "https://registry.npmjs.org/devtools/-/devtools-8.3.11.tgz",
diff --git a/package.json b/package.json
index 840cbcd3..6207d83a 100644
--- a/package.json
+++ b/package.json
@@ -439,6 +439,7 @@
     "@vercel/frameworks": "^1.2.4",
     "@vscode/vscode-languagedetection": "^1.0.22",
     "@vscode/webview-ui-toolkit": "^1.2.1",
+    "detect-port": "^1.5.1",
     "filenamify": "^5.1.1",
     "get-port": "^6.1.2",
     "got": "^12.5.3",
diff --git a/src/extension/server/runmeServer.ts b/src/extension/server/runmeServer.ts
index 04d1888c..712355c1 100644
--- a/src/extension/server/runmeServer.ts
+++ b/src/extension/server/runmeServer.ts
@@ -8,6 +8,7 @@ import { SERVER_ADDRESS } from '../../constants'
 import { enableServerLogs, getBinaryPath, getPortNumber } from '../../utils/configuration'
 import { initParserClient } from '../grpc/client'
 import { DeserializeRequest } from '../grpc/serializerTypes'
+import { isPortAvailable } from '../utils'
 
 import RunmeServerError from './runmeServerError'
 
@@ -23,29 +24,27 @@ export interface IServerConfig {
 
 class RunmeServer implements Disposable {
 
-    #runningPort: number
+    #port: number
     #process: ChildProcessWithoutNullStreams | undefined
     #binaryPath: Uri
     #retryOnFailure: boolean
     #maxNumberOfIntents: number
     #loggingEnabled: boolean
     #intent: number
-    #address: string
     #acceptsIntents: number
     #acceptsInterval: number
     events: EventEmitter
 
     constructor(extBasePath: Uri, options: IServerConfig) {
-        this.#runningPort = getPortNumber()
-        this.#loggingEnabled = enableServerLogs()
-        this.#binaryPath = getBinaryPath(extBasePath, process.platform)
-        this.#retryOnFailure = options.retryOnFailure || false
-        this.#maxNumberOfIntents = options.maxNumberOfIntents
-        this.#intent = 0
-        this.#address = `${SERVER_ADDRESS}:${this.#runningPort}`
-        this.#acceptsIntents = options.acceptsConnection?.intents || 50
-        this.#acceptsInterval = options.acceptsConnection?.interval || 200
-        this.events = new EventEmitter()
+      this.#port = getPortNumber()
+      this.#loggingEnabled = enableServerLogs()
+      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
+      this.events = new EventEmitter()
     }
 
     dispose() {
@@ -69,13 +68,11 @@ class RunmeServer implements Disposable {
       }
     }
 
-    async start(): Promise<string | RunmeServerError> {
-        const running = await this.isRunning()
-        if (running) {
-          console.log(`[Runme] Server already running on addr ${this.#address}`)
-          return this.#address
-        }
+    private address() {
+      return `${SERVER_ADDRESS}:${this.#port}`
+    }
 
+    async start(): Promise<string | RunmeServerError> {
         const binaryLocation = this.#binaryPath.fsPath
 
         const binaryExists = await fs.access(binaryLocation)
@@ -90,10 +87,14 @@ class RunmeServer implements Disposable {
             throw new RunmeServerError('Cannot find server binary file')
         }
 
+        while (!(await isPortAvailable(this.#port))) {
+          this.#port++
+        }
+
         this.#process = spawn(binaryLocation, [
             'server',
             '--address',
-            this.#address
+            this.address()
         ])
 
         this.#process.on('close', () => {
@@ -105,7 +106,7 @@ class RunmeServer implements Disposable {
 
 
         this.#process.stderr.once('data', () => {
-            console.log(`[Runme] Server process #${this.#process?.pid} started`)
+            console.log(`[Runme] Server process #${this.#process?.pid} started on port ${this.#port}`)
         })
 
         this.#process.stderr.on('data', (data) => {
@@ -119,7 +120,7 @@ class RunmeServer implements Disposable {
                 const msg = data.toString()
                 try {
                     const log = JSON.parse(msg)
-                    if (log.addr === this.#address) {
+                    if (log.addr === this.address()) {
                         return resolve(log.addr)
                     }
                 } catch (err: any) {
@@ -170,6 +171,10 @@ class RunmeServer implements Disposable {
         await this.acceptsConnection()
         return addr
     }
+
+    private _port() {
+      return this.#port
+    }
 }
 
 export default RunmeServer
diff --git a/src/extension/utils.ts b/src/extension/utils.ts
index 5d7dcd6b..e5dae55a 100644
--- a/src/extension/utils.ts
+++ b/src/extension/utils.ts
@@ -12,6 +12,7 @@ import vscode, {
   NotebookCellOutput,
 } from 'vscode'
 import { v5 as uuidv5 } from 'uuid'
+import getPort from 'get-port'
 
 import { CellAnnotations, CellAnnotationsErrorResult, Serializer } from '../types'
 import { SafeCellAnnotationsSchema, CellAnnotationsSchema } from '../schema'
@@ -310,3 +311,7 @@ export function replaceOutput(
 export function getGrpcHost() {
   return `${SERVER_ADDRESS}:${getPortNumber()}`
 }
+
+export async function isPortAvailable(port: number): Promise<boolean> {
+  return (await getPort({ port })) === port
+}


Test patch:
diff --git a/tests/extension/__snapshots__/utils.test.ts.snap b/tests/extension/__snapshots__/utils.test.ts.snap
index 1fbfd81b..4ad12ef9 100644
--- a/tests/extension/__snapshots__/utils.test.ts.snap
+++ b/tests/extension/__snapshots__/utils.test.ts.snap
@@ -1,4 +1,4 @@
-// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
+// Vitest Snapshot v1
 
 exports[`getCmdShellSeq > complex wrapped 1`] = `"set -e -o pipefail; curl \\"https://api-us-west-2.graphcms.com/v2/cksds5im94b3w01xq4hfka1r4/master?query=$(deno run -A query.ts)\\" --compressed 2>/dev/null | jq -r '.[].posts[] | \\"(.title) - by (.authors[0].name), id: (.id)\\"'"`;
 
diff --git a/tests/extension/server/runmeServer.test.ts b/tests/extension/server/runmeServer.test.ts
index ea601f52..b98180d5 100644
--- a/tests/extension/server/runmeServer.test.ts
+++ b/tests/extension/server/runmeServer.test.ts
@@ -1,24 +1,35 @@
+import fs from 'node:fs/promises'
+
 import { suite, test, expect, vi, beforeEach } from 'vitest'
 import { Uri, workspace } from 'vscode'
 
+// eslint-disable-next-line import/order
+import { isPortAvailable } from '../../../src/extension/utils'
+
 vi.mock('vscode')
 vi.mock('../../../src/extension/grpc/client', () => ({ initParserClient: vi.fn() }))
 
+vi.mock('../../../src/extension/utils', () => ({
+  isPortAvailable: vi.fn(async () => true),
+}))
+
+vi.mock('node:fs/promises', async () => ({
+  default: {
+      access: vi.fn().mockResolvedValue(false),
+      stat: vi.fn().mockResolvedValue({
+          isFile: vi.fn().mockReturnValue(false)
+      })
+  }
+}))
+
+vi.mock('node:child_process', async () => ({
+  spawn: vi.fn(),
+}))
+
 import Server from '../../../src/extension/server/runmeServer'
 import RunmeServerError from '../../../src/extension/server/runmeServerError'
 
 suite('Runme server spawn process', () => {
-    beforeEach(() => {
-        vi.mock('node:fs/promises', async () => ({
-            default: {
-                access: vi.fn().mockResolvedValue(false),
-                stat: vi.fn().mockResolvedValue({
-                    isFile: vi.fn().mockReturnValue(false)
-                })
-            }
-        }))
-    })
-
     const configValues = {
         binaryPath: 'bin/runme'
     }
@@ -38,6 +49,27 @@ suite('Runme server spawn process', () => {
         await expect(server.launch()).rejects.toBeInstanceOf(RunmeServerError)
         expect(serverLaunchSpy).toBeCalledTimes(3)
     })
+
+    test('Should increment until port is available', async () => {
+      const server = new Server(
+        Uri.file('/Users/user/.vscode/extension/stateful.runme'),
+        {
+          retryOnFailure: true,
+          maxNumberOfIntents: 2,
+        }
+      )
+
+      vi.mocked(fs.access).mockResolvedValueOnce()
+      vi.mocked(fs.stat).mockResolvedValueOnce({
+        isFile: vi.fn().mockReturnValue(true)
+      } as any)
+
+      vi.mocked(isPortAvailable).mockResolvedValueOnce(false)
+      const port = server['_port']()
+      await expect(server.launch()).rejects.toBeInstanceOf(RunmeServerError)
+
+      expect(server['_port']()).toStrictEqual(port + 1)
+    })
 })
 
 suite('Runme server accept connections', () => {