fix: Improve breakpoint handling with multiple connections #606

Request breakpoint info after initialization.
Sync breakpoints to Xdebug connection from internal BreakpointManager state.
Show breakpoints as verified if there are no connections.
Remove _waitingConnections map because the ConfigurationDone handler is not in use anymore.
Cleanup _statuses and _breakpointAdapters to prevent leaks.
Fix typos.
Fix test because the order of startup events changed.

Co-authored-by: Damjan Cvetko <[email protected]>

Author: zobo

CHANGELOG.md

@@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [1.16.1]
+
+### Fixed
+
+- Do not request all breakpoints on every new Xdebug connection. Use internal BreakpointManager state.
+- Show breakpoints as verified when there are no connections.
+
 ## [1.16.0]
 
 ### Added

src/breakpoints.ts

@@ -58,7 +58,7 @@ export class BreakpointManager extends EventEmitter {
             }
 
             let vscodeBreakpoint: VSCodeDebugProtocol.Breakpoint = {
-                verified: false,
+                verified: this.listeners('add').length === 0,
                 line: sourceBreakpoint.line,
                 source: source,
                 id: this._nextId++,
@@ -93,7 +93,7 @@ export class BreakpointManager extends EventEmitter {
         filters.forEach(filter => {
             let xdebugBreakpoint: xdebug.Breakpoint = new xdebug.ExceptionBreakpoint(filter)
             let vscodeBreakpoint: VSCodeDebugProtocol.Breakpoint = {
-                verified: false,
+                verified: this.listeners('add').length === 0,
                 id: this._nextId++,
             }
             this._exceptionBreakpoints.set(vscodeBreakpoint.id!, xdebugBreakpoint)
@@ -130,7 +130,7 @@ export class BreakpointManager extends EventEmitter {
             )
 
             let vscodeBreakpoint: VSCodeDebugProtocol.Breakpoint = {
-                verified: false,
+                verified: this.listeners('add').length === 0,
                 id: this._nextId++,
             }
             this._callBreakpoints.set(vscodeBreakpoint.id!, xdebugBreakpoint)
@@ -153,6 +153,22 @@ export class BreakpointManager extends EventEmitter {
         // this will trigger a process on all adapters
         this.emit('process')
     }
+
+    public getAll(): Map<number, xdebug.Breakpoint> {
+        let toAdd = new Map<number, xdebug.Breakpoint>()
+        for (const [_, lbp] of this._lineBreakpoints) {
+            for (const [id, bp] of lbp) {
+                toAdd.set(id, bp)
+            }
+        }
+        for (const [id, bp] of this._exceptionBreakpoints) {
+            toAdd.set(id, bp)
+        }
+        for (const [id, bp] of this._callBreakpoints) {
+            toAdd.set(id, bp)
+        }
+        return toAdd
+    }
 }
 
 interface AdapterBreakpoint {
@@ -170,7 +186,7 @@ export declare interface BreakpointAdapter {
 
 /**
  * Listens to changes from BreakpointManager and delivers them their own Xdebug Connection.
- * If DBGp connection is busy, trach changes locally.
+ * If DBGp connection is busy, track changes locally.
  */
 export class BreakpointAdapter extends EventEmitter {
     private _connection: xdebug.Connection
@@ -179,18 +195,19 @@ export class BreakpointAdapter extends EventEmitter {
     private _queue: (() => void)[] = []
     private _executing = false
 
-    constructor(connecton: xdebug.Connection, breakpointManager: BreakpointManager) {
+    constructor(connection: xdebug.Connection, breakpointManager: BreakpointManager) {
         super()
-        this._connection = connecton
+        this._connection = connection
         this._breakpointManager = breakpointManager
+        this._add(breakpointManager.getAll())
         // listeners
         this._breakpointManager.on('add', this._add)
         this._breakpointManager.on('remove', this._remove)
-        this._breakpointManager.on('process', this._process)
+        this._breakpointManager.on('process', this.process)
         this._connection.on('close', (error?: Error) => {
             this._breakpointManager.off('add', this._add)
             this._breakpointManager.off('remove', this._remove)
-            this._breakpointManager.off('process', this._process)
+            this._breakpointManager.off('process', this.process)
         })
         this._connection.on('notify_breakpoint_resolved', this._notify)
     }
@@ -238,7 +255,17 @@ export class BreakpointAdapter extends EventEmitter {
         }
     }
 
-    protected _process = async (): Promise<void> => {
+    private _processPromise: Promise<void>
+
+    public process = (): Promise<void> => {
+        if (this._executing) {
+            return this._processPromise
+        }
+        this._processPromise = this.__process()
+        return this._processPromise
+    }
+
+    protected __process = async (): Promise<void> => {
         if (this._executing) {
             // Protect from re-entry
             return
@@ -254,7 +281,7 @@ export class BreakpointAdapter extends EventEmitter {
                 f()
             }
 
-            // do not execute netowrk operations until network channel available
+            // do not execute network operations until network channel available
             if (this._connection.isPendingExecuteCommand) {
                 return
             }
@@ -311,9 +338,9 @@ export class BreakpointAdapter extends EventEmitter {
             this._executing = false
         }
 
-        // If there were any concurent chanegs to the op-queue, rerun processing right away
+        // If there were any concurrent changes to the op-queue, rerun processing right away
         if (this._queue.length > 0) {
-            this._process()
+            return await this.__process()
         }
     }
 }

src/phpDebug.ts

@@ -76,7 +76,7 @@ interface LaunchRequestArguments extends VSCodeDebugProtocol.LaunchRequestArgume
     program?: string
     /** Optional arguments passed to the debuggee. */
     args?: string[]
-    /** Launch the debuggee in this working directory (specified as an absolute path). If omitted the debuggee is lauched in its own directory. */
+    /** Launch the debuggee in this working directory (specified as an absolute path). If omitted the debuggee is launched in its own directory. */
     cwd?: string
     /** Absolute path to the runtime executable to be used. Default is the runtime executable on the PATH. */
     runtimeExecutable?: string
@@ -105,9 +105,6 @@ class PhpDebugSession extends vscode.DebugSession {
      */
     private _connections = new Map<number, xdebug.Connection>()
 
-    /** A set of connections which are not yet running and are waiting for configurationDoneRequest */
-    private _waitingConnections = new Set<xdebug.Connection>()
-
     /** A counter for unique source IDs */
     private _sourceIdCounter = 1
 
@@ -147,6 +144,9 @@ class PhpDebugSession extends vscode.DebugSession {
     /** Breakpoint Manager to map VS Code to Xdebug breakpoints */
     private _breakpointManager = new BreakpointManager()
 
+    /** Breakpoint Adapters */
+    private _breakpointAdapters = new Map<xdebug.Connection, BreakpointAdapter>()
+
     public constructor() {
         super()
         this.setDebuggerColumnsStartAt1(true)
@@ -188,6 +188,8 @@ class PhpDebugSession extends vscode.DebugSession {
             supportTerminateDebuggee: true,
         }
         this.sendResponse(response)
+        // request breakpoints right away
+        this.sendEvent(new vscode.InitializedEvent())
     }
 
     protected attachRequest(
@@ -271,7 +273,6 @@ class PhpDebugSession extends vscode.DebugSession {
                             this.sendEvent(new vscode.OutputEvent('new connection ' + connection.id + '\n'), true)
                         }
                         this._connections.set(connection.id, connection)
-                        this._waitingConnections.add(connection)
                         const disposeConnection = (error?: Error) => {
                             if (this._connections.has(connection.id)) {
                                 if (args.log) {
@@ -288,7 +289,8 @@ class PhpDebugSession extends vscode.DebugSession {
                                 this.sendEvent(new vscode.ThreadEvent('exited', connection.id))
                                 connection.close()
                                 this._connections.delete(connection.id)
-                                this._waitingConnections.delete(connection)
+                                this._statuses.delete(connection)
+                                this._breakpointAdapters.delete(connection)
                             }
                         }
                         connection.on('warning', (warning: string) => {
@@ -336,8 +338,19 @@ class PhpDebugSession extends vscode.DebugSession {
 
                         let bpa = new BreakpointAdapter(connection, this._breakpointManager)
                         bpa.on('dapEvent', event => this.sendEvent(event))
-                        // request breakpoints from VS Code
-                        await this.sendEvent(new vscode.InitializedEvent())
+                        this._breakpointAdapters.set(connection, bpa)
+                        // sync breakpoints to connection
+                        await bpa.process()
+                        let xdebugResponse: xdebug.StatusResponse
+                        // either tell VS Code we stopped on entry or run the script
+                        if (this._args.stopOnEntry) {
+                            // do one step to the first statement
+                            this._hasStoppedOnEntry = false
+                            xdebugResponse = await connection.sendStepIntoCommand()
+                        } else {
+                            xdebugResponse = await connection.sendRunCommand()
+                        }
+                        this._checkStatus(xdebugResponse)
                     } catch (error) {
                         this.sendEvent(
                             new vscode.OutputEvent((error instanceof Error ? error.message : error) + '\n', 'stderr')
@@ -383,9 +396,16 @@ class PhpDebugSession extends vscode.DebugSession {
             this._checkStatus(response)
         } else if (response.status === 'stopped') {
             this._connections.delete(connection.id)
+            this._statuses.delete(connection)
+            this._breakpointAdapters.delete(connection)
             this.sendEvent(new vscode.ThreadEvent('exited', connection.id))
             connection.close()
         } else if (response.status === 'break') {
+            // First sync breakpoints
+            let bpa = this._breakpointAdapters.get(connection)
+            if (bpa) {
+                await bpa.process()
+            }
             // StoppedEvent reason can be 'step', 'breakpoint', 'exception' or 'pause'
             let stoppedEventReason: 'step' | 'breakpoint' | 'exception' | 'pause' | 'entry'
             let exceptionText: string | undefined
@@ -418,7 +438,6 @@ class PhpDebugSession extends vscode.DebugSession {
             )
             event.body.allThreadsStopped = false
             this.sendEvent(event)
-            this._breakpointManager.process()
         }
     }
 
@@ -530,35 +549,10 @@ class PhpDebugSession extends vscode.DebugSession {
         response: VSCodeDebugProtocol.ConfigurationDoneResponse,
         args: VSCodeDebugProtocol.ConfigurationDoneArguments
     ) {
-        let xdebugResponses: xdebug.StatusResponse[] = []
-        try {
-            xdebugResponses = await Promise.all<xdebug.StatusResponse>(
-                Array.from(this._waitingConnections).map(connection => {
-                    this._waitingConnections.delete(connection)
-                    // either tell VS Code we stopped on entry or run the script
-                    if (this._args.stopOnEntry) {
-                        // do one step to the first statement
-                        this._hasStoppedOnEntry = false
-                        return connection.sendStepIntoCommand()
-                    } else {
-                        return connection.sendRunCommand()
-                    }
-                })
-            )
-        } catch (error) {
-            this.sendErrorResponse(response, <Error>error)
-            for (const response of xdebugResponses) {
-                this._checkStatus(response)
-            }
-            return
-        }
         this.sendResponse(response)
-        for (const response of xdebugResponses) {
-            this._checkStatus(response)
-        }
     }
 
-    /** Executed after a successfull launch or attach request and after a ThreadEvent */
+    /** Executed after a successful launch or attach request and after a ThreadEvent */
     protected threadsRequest(response: VSCodeDebugProtocol.ThreadsResponse): void {
         // PHP doesn't have threads, but it may have multiple requests in parallel.
         // Think about a website that makes multiple, parallel AJAX requests to your PHP backend.
@@ -952,7 +946,8 @@ class PhpDebugSession extends vscode.DebugSession {
                     }
                     await connection.close()
                     this._connections.delete(id)
-                    this._waitingConnections.delete(connection)
+                    this._statuses.delete(connection)
+                    this._breakpointAdapters.delete(connection)
                 })
             )
             // If listening for connections, close server

src/test/adapter.ts

@@ -367,8 +367,7 @@ describe('PHP Debug Adapter', () => {
             const program = path.join(TEST_PROJECT, 'function.php')
 
             it('should stop on a function breakpoint', async () => {
-                await client.launch({ program })
-                await client.waitForEvent('initialized')
+                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
                 const breakpoint = (
                     await client.setFunctionBreakpointsRequest({
                         breakpoints: [{ name: 'a_function' }],