fix: fixed breakpoint and launch initialization order. (#608)

* Fixed breakpoint and launch initialization order. * Improve order or Launch, Initialize and breakpoint processing. * Optimize feature negotiation for known Xdebug version. Co-authored-by: Damjan Cvetko <damjan.cvetko@monotek.net>
xdebug · Jul 29, 2021 · 4eda887 · 4eda887
1 parent 700113f
commit 4eda887
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 46 deletions.
diff --git a/CHANGELOG.md b/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.2]
+
+## Fixed
+
+- Fixed breakpoint and launch initialization order.
+- Optimize feature negotiation for known Xdebug version.
+
 ## [1.16.1]
 
 ### Fixed

diff --git a/package.json b/package.json
@@ -394,4 +394,4 @@
       }
     ]
   }
-}
+}
diff --git a/src/phpDebug.ts b/src/phpDebug.ts
@@ -12,6 +12,7 @@ import { Terminal } from './terminal'
 import { convertClientPathToDebugger, convertDebuggerPathToClient } from './paths'
 import minimatch = require('minimatch')
 import { BreakpointManager, BreakpointAdapter } from './breakpoints'
+import * as semver from 'semver'
 
 if (process.env['VSCODE_NLS_CONFIG']) {
     try {
@@ -147,6 +148,12 @@ class PhpDebugSession extends vscode.DebugSession {
     /** Breakpoint Adapters */
     private _breakpointAdapters = new Map<xdebug.Connection, BreakpointAdapter>()
 
+    /** the promise that gets resolved once we receive the done request */
+    private _donePromise: Promise<void>
+
+    /** resolves the done promise */
+    private _donePromiseResolveFn: () => any
+
     public constructor() {
         super()
         this.setDebuggerColumnsStartAt1(true)
@@ -188,8 +195,6 @@ class PhpDebugSession extends vscode.DebugSession {
             supportTerminateDebuggee: true,
         }
         this.sendResponse(response)
-        // request breakpoints right away
-        this.sendEvent(new vscode.InitializedEvent())
     }
 
     protected attachRequest(
@@ -210,6 +215,11 @@ class PhpDebugSession extends vscode.DebugSession {
             args.pathMappings = pathMappings
         }
         this._args = args
+
+        this._donePromise = new Promise<void>((resolve, reject) => {
+            this._donePromiseResolveFn = resolve
+        })
+
         /** launches the script as CLI */
         const launchScript = async (port: number) => {
             // check if program exists
@@ -304,19 +314,30 @@ class PhpDebugSession extends vscode.DebugSession {
                                 this.sendEvent(new vscode.OutputEvent(log), true)
                             }
                         })
-                        await connection.waitForInitPacket()
+                        const initPacket = await connection.waitForInitPacket()
 
                         // support for breakpoints
-                        let feat_rb = await connection.sendFeatureGetCommand('resolved_breakpoints')
-                        if (feat_rb.supported === '1') {
+                        let feat: xdebug.FeatureGetResponse
+                        const supportedEngine =
+                            initPacket.engineName === 'Xdebug' && semver.gte(initPacket.engineVersion, '3.0.0')
+                        if (
+                            supportedEngine ||
+                            ((feat = await connection.sendFeatureGetCommand('resolved_breakpoints')) &&
+                                feat.supported === '1')
+                        ) {
                             await connection.sendFeatureSetCommand('resolved_breakpoints', '1')
                         }
-                        let feat_no = await connection.sendFeatureGetCommand('notify_ok')
-                        if (feat_no.supported === '1') {
+                        if (
+                            supportedEngine ||
+                            ((feat = await connection.sendFeatureGetCommand('notify_ok')) && feat.supported === '1')
+                        ) {
                             await connection.sendFeatureSetCommand('notify_ok', '1')
                         }
-                        let feat_ep = await connection.sendFeatureGetCommand('extended_properties')
-                        if (feat_ep.supported === '1') {
+                        if (
+                            supportedEngine ||
+                            ((feat = await connection.sendFeatureGetCommand('extended_properties')) &&
+                                feat.supported === '1')
+                        ) {
                             await connection.sendFeatureSetCommand('extended_properties', '1')
                         }
 
@@ -336,6 +357,9 @@ class PhpDebugSession extends vscode.DebugSession {
 
                         this.sendEvent(new vscode.ThreadEvent('started', connection.id))
 
+                        // wait for all breakpoints
+                        await this._donePromise
+
                         let bpa = new BreakpointAdapter(connection, this._breakpointManager)
                         bpa.on('dapEvent', event => this.sendEvent(event))
                         this._breakpointAdapters.set(connection, bpa)
@@ -382,6 +406,8 @@ class PhpDebugSession extends vscode.DebugSession {
             return
         }
         this.sendResponse(response)
+        // request breakpoints
+        this.sendEvent(new vscode.InitializedEvent())
     }
 
     /**
@@ -550,6 +576,7 @@ class PhpDebugSession extends vscode.DebugSession {
         args: VSCodeDebugProtocol.ConfigurationDoneArguments
     ) {
         this.sendResponse(response)
+        this._donePromiseResolveFn()
     }
 
     /** Executed after a successful launch or attach request and after a ThreadEvent */

diff --git a/src/test/adapter.ts b/src/test/adapter.ts
@@ -34,7 +34,9 @@ describe('PHP Debug Adapter', () => {
         const program = path.join(TEST_PROJECT, 'hello_world.php')
 
         it('should error on non-existing file', () =>
-            assert.isRejected(client.launch({ program: 'thisfiledoesnotexist.php' })))
+            assert.isRejected(
+                Promise.all([client.launch({ program: 'thisfiledoesnotexist.php' }), client.configurationSequence()])
+            ))
 
         it('should run program to the end', () =>
             Promise.all([
@@ -55,6 +57,7 @@ describe('PHP Debug Adapter', () => {
         it('should not stop if launched without debugging', () =>
             Promise.all([
                 client.launch({ program, stopOnEntry: true, noDebug: true }),
+                client.configurationSequence(),
                 client.waitForEvent('terminated'),
             ]))
     })
@@ -70,7 +73,7 @@ describe('PHP Debug Adapter', () => {
         it('should error on pause request', () => assert.isRejected(client.pauseRequest({ threadId: 1 })))
 
         it('should handle disconnect', async () => {
-            await Promise.all([client.launch({ program, stopOnEntry: true }), client.waitForEvent('initialized')])
+            await Promise.all([client.launch({ program, stopOnEntry: true }), client.configurationSequence()])
             await client.disconnectRequest()
         })
     })
@@ -113,20 +116,18 @@ describe('PHP Debug Adapter', () => {
 
         describe('line breakpoints', () => {
             async function testBreakpointHit(program: string, line: number): Promise<void> {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 const breakpoint = (
                     await client.setBreakpointsRequest({
                         breakpoints: [{ line }],
                         source: { path: program },
                     })
                 ).body.breakpoints[0]
-                await waitForBreakpointUpdate(breakpoint)
+                await client.configurationDoneRequest(), await waitForBreakpointUpdate(breakpoint)
                 assert.isTrue(breakpoint.verified, 'breakpoint verification mismatch: verified')
                 assert.equal(breakpoint.line, line, 'breakpoint verification mismatch: line')
-                await Promise.all([
-                    client.configurationDoneRequest(),
-                    assertStoppedLocation('breakpoint', program, line),
-                ])
+                await assertStoppedLocation('breakpoint', program, line)
             }
 
             it('should stop on a breakpoint', () => testBreakpointHit(program, 4))
@@ -137,15 +138,16 @@ describe('PHP Debug Adapter', () => {
             it('should stop on a breakpoint identical to the entrypoint', () => testBreakpointHit(program, 3))
 
             it('should support removing a breakpoint', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 // set two breakpoints
                 let breakpoints = (
                     await client.setBreakpointsRequest({
                         breakpoints: [{ line: 3 }, { line: 5 }],
                         source: { path: program },
                     })
                 ).body.breakpoints
-                await waitForBreakpointUpdate(breakpoints[0])
+                await client.configurationDoneRequest(), await waitForBreakpointUpdate(breakpoints[0])
                 await waitForBreakpointUpdate(breakpoints[1])
                 assert.lengthOf(breakpoints, 2)
                 assert.isTrue(breakpoints[0].verified, 'breakpoint verification mismatch: verified')
@@ -177,13 +179,15 @@ describe('PHP Debug Adapter', () => {
             const program = path.join(TEST_PROJECT, 'error.php')
 
             it('should not break on anything if the file matches the ignore pattern', async () => {
-                await Promise.all([client.launch({ program, ignore: ['**/*.*'] }), client.waitForEvent('initialized')])
+                client.launch({ program, ignore: ['**/*.*'] })
+                await client.waitForEvent('initialized')
                 await client.setExceptionBreakpointsRequest({ filters: ['*'] })
                 await Promise.all([client.configurationDoneRequest(), client.waitForEvent('terminated')])
             })
 
             it('should support stopping only on a notice', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 await client.setExceptionBreakpointsRequest({ filters: ['Notice'] })
                 const [, { threadId }] = await Promise.all([
                     client.configurationDoneRequest(),
@@ -193,7 +197,8 @@ describe('PHP Debug Adapter', () => {
             })
 
             it('should support stopping only on a warning', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 await client.setExceptionBreakpointsRequest({ filters: ['Warning'] })
                 const [{ threadId }] = await Promise.all([
                     assertStoppedLocation('exception', program, 9),
@@ -205,7 +210,8 @@ describe('PHP Debug Adapter', () => {
             it('should support stopping only on an error')
 
             it('should support stopping only on an exception', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 await client.setExceptionBreakpointsRequest({ filters: ['Exception'] })
                 const [, { threadId }] = await Promise.all([
                     client.configurationDoneRequest(),
@@ -217,7 +223,8 @@ describe('PHP Debug Adapter', () => {
             // support for stopping on "*" was added in 2.3.0
             if (!process.env['XDEBUG_VERSION'] || semver.gte(process.env['XDEBUG_VERSION'], '2.3.0')) {
                 it('should support stopping on everything', async () => {
-                    await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                    client.launch({ program })
+                    await client.waitForEvent('initialized')
                     await client.setExceptionBreakpointsRequest({ filters: ['*'] })
                     // Notice
                     const [, { threadId }] = await Promise.all([
@@ -244,7 +251,8 @@ describe('PHP Debug Adapter', () => {
             }
 
             it.skip('should report the error in a virtual error scope', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 await client.setExceptionBreakpointsRequest({ filters: ['Notice', 'Warning', 'Exception'] })
                 const [
                     {
@@ -324,20 +332,19 @@ describe('PHP Debug Adapter', () => {
             const program = path.join(TEST_PROJECT, 'variables.php')
 
             it('should stop on a conditional breakpoint when condition is true', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 const bp = (
                     await client.setBreakpointsRequest({
                         breakpoints: [{ line: 10, condition: '$anInt === 123' }],
                         source: { path: program },
                     })
                 ).body.breakpoints[0]
+                await client.configurationDoneRequest()
                 await waitForBreakpointUpdate(bp)
                 assert.equal(bp.verified, true, 'breakpoint verification mismatch: verified')
                 assert.equal(bp.line, 10, 'breakpoint verification mismatch: line')
-                const [, { frame }] = await Promise.all([
-                    client.configurationDoneRequest(),
-                    assertStoppedLocation('breakpoint', program, 10),
-                ])
+                const { frame } = await assertStoppedLocation('breakpoint', program, 10)
                 const result = (
                     await client.evaluateRequest({
                         context: 'watch',
@@ -349,33 +356,37 @@ describe('PHP Debug Adapter', () => {
             })
 
             it('should not stop on a conditional breakpoint when condition is false', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 const bp = (
                     await client.setBreakpointsRequest({
                         breakpoints: [{ line: 10, condition: '$anInt !== 123' }],
                         source: { path: program },
                     })
                 ).body.breakpoints[0]
+                await client.configurationDoneRequest()
                 await waitForBreakpointUpdate(bp)
                 assert.equal(bp.verified, true, 'breakpoint verification mismatch: verified')
                 assert.equal(bp.line, 10, 'breakpoint verification mismatch: line')
-                await Promise.all([client.configurationDoneRequest(), client.waitForEvent('terminated')])
+                await client.waitForEvent('terminated')
             })
         })
 
         describe('function breakpoints', () => {
             const program = path.join(TEST_PROJECT, 'function.php')
 
             it('should stop on a function breakpoint', async () => {
-                await Promise.all([client.launch({ program }), client.waitForEvent('initialized')])
+                client.launch({ program })
+                await client.waitForEvent('initialized')
                 const breakpoint = (
                     await client.setFunctionBreakpointsRequest({
                         breakpoints: [{ name: 'a_function' }],
                     })
                 ).body.breakpoints[0]
+                await client.configurationDoneRequest()
                 await waitForBreakpointUpdate(breakpoint)
                 assert.strictEqual(breakpoint.verified, true)
-                await Promise.all([client.configurationDoneRequest(), assertStoppedLocation('breakpoint', program, 5)])
+                await assertStoppedLocation('breakpoint', program, 5)
             })
         })
     })
@@ -388,16 +399,14 @@ describe('PHP Debug Adapter', () => {
         let constantsScope: DebugProtocol.Scope | undefined
 
         beforeEach(async () => {
-            await Promise.all([
-                client.launch({
-                    program,
-                    xdebugSettings: {
-                        max_data: 10000,
-                        max_children: 100,
-                    },
-                }),
-                client.waitForEvent('initialized'),
-            ])
+            client.launch({
+                program,
+                xdebugSettings: {
+                    max_data: 10000,
+                    max_children: 100,
+                },
+            })
+            await client.waitForEvent('initialized')
             await client.setBreakpointsRequest({ source: { path: program }, breakpoints: [{ line: 19 }] })
             const [, event] = await Promise.all([
                 client.configurationDoneRequest(),

diff --git a/src/xdebugConnection.ts b/src/xdebugConnection.ts
@@ -19,6 +19,8 @@ export class InitPacket {
     connection: Connection
     /** the version of Xdebug */
     engineVersion: string
+    /** the name of the engine */
+    engineName: string
     /**
      * @param  {XMLDocument} document - An XML document to read from
      * @param  {Connection} connection
@@ -30,6 +32,7 @@ export class InitPacket {
         this.protocolVersion = documentElement.getAttribute('protocol_version')!
         this.ideKey = documentElement.getAttribute('idekey')!
         this.engineVersion = documentElement.getElementsByTagName('engine')[0].getAttribute('version')!
+        this.engineName = documentElement.getElementsByTagName('engine')[0].textContent ?? ''
         this.connection = connection
     }
 }
@@ -936,7 +939,7 @@ export class Connection extends DbgpConnection {
         )
     }
 
-    /** Sends a context_get comand */
+    /** Sends a context_get command */
     public async sendContextGetCommand(context: Context): Promise<ContextGetResponse> {
         return new ContextGetResponse(
             await this._enqueueCommand('context_get', `-d ${context.stackFrame.level} -c ${context.id}`),