Skip to content

Commit 7fa2d28

Browse files
jonyofelixfbecker
jonyo
authored and
felixfbecker
committed
fix: avoid deadlock with multiple connections when executing code (#294)
This introduces a new concept in the xDebug connection: "execution commands": these are commands that result in PHP being executed. The way xDebug works, if one of these commands is run, that connection is hung until the PHP code is done executing (until it gets to the next stopping point). By keeping track of when such commands are still in progress, and making it something that the adapter can check, it allows the adapter to specifically code for when the connection is currently hung. Which allows preventing the deadlock situation described in #164 The adapter also now emits `before-execution-command` since it results in possibly going into a hung state for a while. The adapter uses that event to send a `ContinueEvent`, this is just a side advantage that makes the interface more responsive. Before when you click continue, if the PHP code is hung for whatever reason (it is slow, it has a lot to do, etc), it appears that it did not work as it appears to still be stopped on a breakpoint (but it is not, which you can tell by trying to view any of the details in the debug pane). With the change, now when you click continue it immediately changes the process back to "running" state which is more truthful. Since #164 has a lot of comments, here is the **tl;dr:** deadlock explanation: 1. Connection 1 makes a second connection with curl. 2. It sends command to all connections to set breakpoints, and waits for response. 3. The second connection hangs as it is waiting for connection 1 to set breakpoints, connection 1 hangs without setting those breakpoints because it is waiting for connection 2 to respond and can't do anything until that completes. This PR makes it not wait for the breakpoints to be set on a connection if the connection is waiting for code to execute. So now connection 2 can finish initializing and run, no more deadlock. Fixes #164
1 parent 08a5500 commit 7fa2d28

File tree

2 files changed

+128
-77
lines changed

2 files changed

+128
-77
lines changed

‎src/phpDebug.ts‎

Lines changed: 83 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ class PhpDebugSession extends vscode.DebugSession {
238238
this.sendEvent(new vscode.TerminatedEvent())
239239
})
240240
script.on('error', (error: Error) => {
241-
this.sendEvent(new vscode.OutputEvent(error.message + '\n'))
241+
this.sendEvent(new vscode.OutputEvent(util.inspect(error) + '\n'))
242242
})
243243
this._phpProcess = script
244244
}
@@ -279,6 +279,10 @@ class PhpDebugSession extends vscode.DebugSession {
279279
})
280280
connection.on('error', disposeConnection)
281281
connection.on('close', disposeConnection)
282+
connection.on('before-execute-command', () => {
283+
// It is about to start executing PHP code
284+
this.sendEvent(new vscode.ContinuedEvent(connection.id))
285+
})
282286
await connection.waitForInitPacket()
283287

284288
// override features from launch.json
@@ -307,7 +311,7 @@ class PhpDebugSession extends vscode.DebugSession {
307311
}
308312
})
309313
server.on('error', (error: Error) => {
310-
this.sendEvent(new vscode.OutputEvent('ERROR: ' + error.message + '\n', 'stderr'))
314+
this.sendEvent(new vscode.OutputEvent(util.inspect(error) + '\n'))
311315
this.sendErrorResponse(response, <Error>error)
312316
})
313317
server.listen(args.port || 9000, (error: NodeJS.ErrnoException) => (error ? reject(error) : resolve()))
@@ -458,41 +462,43 @@ class PhpDebugSession extends vscode.DebugSession {
458462
// for all connections
459463
await Promise.all(
460464
connections.map(async (connection, connectionIndex) => {
461-
// clear breakpoints for this file
462-
// in the future when VS Code returns the breakpoint IDs it would be better to calculate the diff
463-
const { breakpoints } = await connection.sendBreakpointListCommand()
464-
await Promise.all(
465-
breakpoints
466-
// filter to only include line breakpoints for this file
467-
.filter(
468-
breakpoint =>
469-
breakpoint instanceof xdebug.LineBreakpoint &&
470-
isSameUri(fileUri, breakpoint.fileUri)
471-
)
472-
// remove them
473-
.map(breakpoint => breakpoint.remove())
474-
)
475-
// set new breakpoints
476-
await Promise.all(
477-
xdebugBreakpoints.map(async (breakpoint, index) => {
478-
try {
479-
await connection.sendBreakpointSetCommand(breakpoint)
480-
// only capture each breakpoint once
481-
if (connectionIndex === 0) {
465+
const promise = (async () => {
466+
const { breakpoints } = await connection.sendBreakpointListCommand()
467+
// clear breakpoints for this file
468+
// in the future when VS Code returns the breakpoint IDs it would be better to calculate the diff
469+
await Promise.all(
470+
breakpoints
471+
.filter(
472+
breakpoint =>
473+
breakpoint instanceof xdebug.LineBreakpoint &&
474+
isSameUri(fileUri, breakpoint.fileUri)
475+
)
476+
.map(breakpoint => breakpoint.remove())
477+
)
478+
// set new breakpoints
479+
await Promise.all(
480+
xdebugBreakpoints.map(async (breakpoint, index) => {
481+
try {
482+
await connection.sendBreakpointSetCommand(breakpoint)
482483
vscodeBreakpoints[index] = { verified: true, line: breakpoint.line }
483-
}
484-
} catch (error) {
485-
// only capture each breakpoint once
486-
if (connectionIndex === 0) {
484+
} catch (error) {
487485
vscodeBreakpoints[index] = {
488486
verified: false,
489487
line: breakpoint.line,
490488
message: (<Error>error).message,
491489
}
492490
}
493-
}
494-
})
495-
)
491+
})
492+
)
493+
})()
494+
495+
if (connection.isPendingExecuteCommand) {
496+
// There is a pending execute command which could lock the connection up, so do not
497+
// wait on the response before continuing or it can get into a deadlock
498+
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
499+
} else {
500+
await promise
501+
}
496502
})
497503
)
498504
}
@@ -513,20 +519,26 @@ class PhpDebugSession extends vscode.DebugSession {
513519
const connections = Array.from(this._connections.values())
514520
await Promise.all(
515521
connections.map(async connection => {
516-
// get all breakpoints
517-
const { breakpoints } = await connection.sendBreakpointListCommand()
518-
// remove all exception breakpoints
519-
await Promise.all(
520-
breakpoints
521-
.filter(breakpoint => breakpoint.type === 'exception')
522-
.map(breakpoint => breakpoint.remove())
523-
)
524-
// set new exception breakpoints
525-
await Promise.all(
526-
args.filters.map(filter =>
527-
connection.sendBreakpointSetCommand(new xdebug.ExceptionBreakpoint(filter))
522+
const promise = (async () => {
523+
const { breakpoints } = await connection.sendBreakpointListCommand()
524+
await Promise.all(
525+
breakpoints
526+
.filter(breakpoint => breakpoint.type === 'exception')
527+
.map(breakpoint => breakpoint.remove())
528528
)
529-
)
529+
await Promise.all(
530+
args.filters.map(filter =>
531+
connection.sendBreakpointSetCommand(new xdebug.ExceptionBreakpoint(filter))
532+
)
533+
)
534+
})()
535+
if (connection.isPendingExecuteCommand) {
536+
// There is a pending execute command which could lock the connection up, so do not
537+
// wait on the response before continuing or it can get into a deadlock
538+
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
539+
} else {
540+
await promise
541+
}
530542
})
531543
)
532544
} catch (error) {
@@ -552,35 +564,40 @@ class PhpDebugSession extends vscode.DebugSession {
552564
// for all connections
553565
await Promise.all(
554566
connections.map(async (connection, connectionIndex) => {
555-
// clear breakpoints for this file
556-
const { breakpoints } = await connection.sendBreakpointListCommand()
557-
await Promise.all(
558-
breakpoints
559-
.filter(breakpoint => breakpoint.type === 'call')
560-
.map(breakpoint => breakpoint.remove())
561-
)
562-
// set new breakpoints
563-
await Promise.all(
564-
args.breakpoints.map(async (functionBreakpoint, index) => {
565-
try {
566-
await connection.sendBreakpointSetCommand(
567-
new xdebug.CallBreakpoint(functionBreakpoint.name, functionBreakpoint.condition)
568-
)
569-
// only capture each breakpoint once
570-
if (connectionIndex === 0) {
567+
const promise = (async () => {
568+
const { breakpoints } = await connection.sendBreakpointListCommand()
569+
await Promise.all(
570+
breakpoints
571+
.filter(breakpoint => breakpoint.type === 'call')
572+
.map(breakpoint => breakpoint.remove())
573+
)
574+
await Promise.all(
575+
args.breakpoints.map(async (functionBreakpoint, index) => {
576+
try {
577+
await connection.sendBreakpointSetCommand(
578+
new xdebug.CallBreakpoint(
579+
functionBreakpoint.name,
580+
functionBreakpoint.condition
581+
)
582+
)
571583
vscodeBreakpoints[index] = { verified: true }
572-
}
573-
} catch (error) {
574-
// only capture each breakpoint once
575-
if (connectionIndex === 0) {
584+
} catch (error) {
576585
vscodeBreakpoints[index] = {
577586
verified: false,
578587
message: error instanceof Error ? error.message : error,
579588
}
580589
}
581-
}
582-
})
583-
)
590+
})
591+
)
592+
})()
593+
594+
if (connection.isPendingExecuteCommand) {
595+
// There is a pending execute command which could lock the connection up, so do not
596+
// wait on the response before continuing or it can get into a deadlock
597+
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
598+
} else {
599+
await promise
600+
}
584601
})
585602
)
586603
}

‎src/xdebugConnection.ts‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,8 @@ interface Command {
545545
resolveFn: (response: XMLDocument) => any
546546
/** callback that gets called if an error happened while parsing the response */
547547
rejectFn: (error?: Error) => any
548+
/** whether command results in PHP code being executed or not */
549+
isExecuteCommand: boolean
548550
}
549551

550552
/**
@@ -585,6 +587,15 @@ export class Connection extends DbgpConnection {
585587
*/
586588
private _commandQueue: Command[] = []
587589

590+
private _pendingExecuteCommand = false
591+
/**
592+
* Whether a command was started that executes PHP, which means the connection will be blocked from
593+
* running any additional commands until the execution gets to the next stopping point or exits.
594+
*/
595+
public get isPendingExecuteCommand(): boolean {
596+
return this._pendingExecuteCommand
597+
}
598+
588599
/** Constructs a new connection that uses the given socket to communicate with XDebug. */
589600
constructor(socket: net.Socket) {
590601
super(socket)
@@ -602,6 +613,7 @@ export class Connection extends DbgpConnection {
602613
if (this._pendingCommands.has(transactionId)) {
603614
const command = this._pendingCommands.get(transactionId)!
604615
this._pendingCommands.delete(transactionId)
616+
this._pendingExecuteCommand = false
605617
command.resolveFn(response)
606618
}
607619
if (this._commandQueue.length > 0) {
@@ -624,15 +636,31 @@ export class Connection extends DbgpConnection {
624636
*/
625637
private _enqueueCommand(name: string, args?: string, data?: string): Promise<XMLDocument> {
626638
return new Promise((resolveFn, rejectFn) => {
627-
const command = { name, args, data, resolveFn, rejectFn }
628-
if (this._commandQueue.length === 0 && this._pendingCommands.size === 0) {
629-
this._executeCommand(command)
630-
} else {
631-
this._commandQueue.push(command)
632-
}
639+
this._enqueue({ name, args, data, resolveFn, rejectFn, isExecuteCommand: false })
633640
})
634641
}
635642

643+
/**
644+
* Pushes a new execute command (one that results in executing PHP code) to the queue that will be executed after all the previous
645+
* commands have finished and we received a response.
646+
* If the queue is empty AND there are no pending transactions (meaning we already received a response and XDebug is waiting for
647+
* commands) the command will be executed immediately.
648+
*/
649+
private _enqueueExecuteCommand(name: string, args?: string, data?: string): Promise<XMLDocument> {
650+
return new Promise((resolveFn, rejectFn) => {
651+
this._enqueue({ name, args, data, resolveFn, rejectFn, isExecuteCommand: true })
652+
})
653+
}
654+
655+
/** Adds the given command to the queue, or executes immediately if no commands are currently being processed. */
656+
private _enqueue(command: Command): void {
657+
if (this._commandQueue.length === 0 && this._pendingCommands.size === 0) {
658+
this._executeCommand(command)
659+
} else {
660+
this._commandQueue.push(command)
661+
}
662+
}
663+
636664
/**
637665
* Sends a command to XDebug with a new transaction ID and calls the callback on the command. This can
638666
* only be called when XDebug can actually accept commands, which is after we received a response for the
@@ -649,8 +677,14 @@ export class Connection extends DbgpConnection {
649677
}
650678
commandString += '\0'
651679
const data = iconv.encode(commandString, ENCODING)
652-
await this.write(data)
653680
this._pendingCommands.set(transactionId, command)
681+
this._pendingExecuteCommand = command.isExecuteCommand
682+
if (this._pendingExecuteCommand) {
683+
// Since PHP execution commands block anything on the connection until it is
684+
// done executing, emit that the connection is about to go into such a locked state
685+
this.emit('before-execute-command')
686+
}
687+
await this.write(data)
654688
}
655689

656690
public close() {
@@ -752,22 +786,22 @@ export class Connection extends DbgpConnection {
752786

753787
/** sends a run command */
754788
public async sendRunCommand(): Promise<StatusResponse> {
755-
return new StatusResponse(await this._enqueueCommand('run'), this)
789+
return new StatusResponse(await this._enqueueExecuteCommand('run'), this)
756790
}
757791

758792
/** sends a step_into command */
759793
public async sendStepIntoCommand(): Promise<StatusResponse> {
760-
return new StatusResponse(await this._enqueueCommand('step_into'), this)
794+
return new StatusResponse(await this._enqueueExecuteCommand('step_into'), this)
761795
}
762796

763797
/** sends a step_over command */
764798
public async sendStepOverCommand(): Promise<StatusResponse> {
765-
return new StatusResponse(await this._enqueueCommand('step_over'), this)
799+
return new StatusResponse(await this._enqueueExecuteCommand('step_over'), this)
766800
}
767801

768802
/** sends a step_out command */
769803
public async sendStepOutCommand(): Promise<StatusResponse> {
770-
return new StatusResponse(await this._enqueueCommand('step_out'), this)
804+
return new StatusResponse(await this._enqueueExecuteCommand('step_out'), this)
771805
}
772806

773807
/** sends a stop command */

0 commit comments

Comments
 (0)