Skip to content

Commit

Permalink
fix: avoid deadlock with multiple connections when executing code (#294)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jonyo @felixfbecker
jonyo authored and felixfbecker committed Aug 28, 2018
1 parent 08a5500 commit 7fa2d28
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 77 deletions.
149 changes: 83 additions & 66 deletions src/phpDebug.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ class PhpDebugSession extends vscode.DebugSession {
this.sendEvent(new vscode.TerminatedEvent())
})
script.on('error', (error: Error) => {
this.sendEvent(new vscode.OutputEvent(error.message + '\n'))
this.sendEvent(new vscode.OutputEvent(util.inspect(error) + '\n'))
})
this._phpProcess = script
}
Expand Down Expand Up @@ -279,6 +279,10 @@ class PhpDebugSession extends vscode.DebugSession {
})
connection.on('error', disposeConnection)
connection.on('close', disposeConnection)
connection.on('before-execute-command', () => {
// It is about to start executing PHP code
this.sendEvent(new vscode.ContinuedEvent(connection.id))
})
await connection.waitForInitPacket()

// override features from launch.json
Expand Down Expand Up @@ -307,7 +311,7 @@ class PhpDebugSession extends vscode.DebugSession {
}
})
server.on('error', (error: Error) => {
this.sendEvent(new vscode.OutputEvent('ERROR: ' + error.message + '\n', 'stderr'))
this.sendEvent(new vscode.OutputEvent(util.inspect(error) + '\n'))
this.sendErrorResponse(response, <Error>error)
})
server.listen(args.port || 9000, (error: NodeJS.ErrnoException) => (error ? reject(error) : resolve()))
Expand Down Expand Up @@ -458,41 +462,43 @@ class PhpDebugSession extends vscode.DebugSession {
// for all connections
await Promise.all(
connections.map(async (connection, connectionIndex) => {
// clear breakpoints for this file
// in the future when VS Code returns the breakpoint IDs it would be better to calculate the diff
const { breakpoints } = await connection.sendBreakpointListCommand()
await Promise.all(
breakpoints
// filter to only include line breakpoints for this file
.filter(
breakpoint =>
breakpoint instanceof xdebug.LineBreakpoint &&
isSameUri(fileUri, breakpoint.fileUri)
)
// remove them
.map(breakpoint => breakpoint.remove())
)
// set new breakpoints
await Promise.all(
xdebugBreakpoints.map(async (breakpoint, index) => {
try {
await connection.sendBreakpointSetCommand(breakpoint)
// only capture each breakpoint once
if (connectionIndex === 0) {
const promise = (async () => {
const { breakpoints } = await connection.sendBreakpointListCommand()
// clear breakpoints for this file
// in the future when VS Code returns the breakpoint IDs it would be better to calculate the diff
await Promise.all(
breakpoints
.filter(
breakpoint =>
breakpoint instanceof xdebug.LineBreakpoint &&
isSameUri(fileUri, breakpoint.fileUri)
)
.map(breakpoint => breakpoint.remove())
)
// set new breakpoints
await Promise.all(
xdebugBreakpoints.map(async (breakpoint, index) => {
try {
await connection.sendBreakpointSetCommand(breakpoint)
vscodeBreakpoints[index] = { verified: true, line: breakpoint.line }
}
} catch (error) {
// only capture each breakpoint once
if (connectionIndex === 0) {
} catch (error) {
vscodeBreakpoints[index] = {
verified: false,
line: breakpoint.line,
message: (<Error>error).message,
}
}
}
})
)
})
)
})()

if (connection.isPendingExecuteCommand) {
// There is a pending execute command which could lock the connection up, so do not
// wait on the response before continuing or it can get into a deadlock
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
} else {
await promise
}
})
)
}
Expand All @@ -513,20 +519,26 @@ class PhpDebugSession extends vscode.DebugSession {
const connections = Array.from(this._connections.values())
await Promise.all(
connections.map(async connection => {
// get all breakpoints
const { breakpoints } = await connection.sendBreakpointListCommand()
// remove all exception breakpoints
await Promise.all(
breakpoints
.filter(breakpoint => breakpoint.type === 'exception')
.map(breakpoint => breakpoint.remove())
)
// set new exception breakpoints
await Promise.all(
args.filters.map(filter =>
connection.sendBreakpointSetCommand(new xdebug.ExceptionBreakpoint(filter))
const promise = (async () => {
const { breakpoints } = await connection.sendBreakpointListCommand()
await Promise.all(
breakpoints
.filter(breakpoint => breakpoint.type === 'exception')
.map(breakpoint => breakpoint.remove())
)
)
await Promise.all(
args.filters.map(filter =>
connection.sendBreakpointSetCommand(new xdebug.ExceptionBreakpoint(filter))
)
)
})()
if (connection.isPendingExecuteCommand) {
// There is a pending execute command which could lock the connection up, so do not
// wait on the response before continuing or it can get into a deadlock
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
} else {
await promise
}
})
)
} catch (error) {
Expand All @@ -552,35 +564,40 @@ class PhpDebugSession extends vscode.DebugSession {
// for all connections
await Promise.all(
connections.map(async (connection, connectionIndex) => {
// clear breakpoints for this file
const { breakpoints } = await connection.sendBreakpointListCommand()
await Promise.all(
breakpoints
.filter(breakpoint => breakpoint.type === 'call')
.map(breakpoint => breakpoint.remove())
)
// set new breakpoints
await Promise.all(
args.breakpoints.map(async (functionBreakpoint, index) => {
try {
await connection.sendBreakpointSetCommand(
new xdebug.CallBreakpoint(functionBreakpoint.name, functionBreakpoint.condition)
)
// only capture each breakpoint once
if (connectionIndex === 0) {
const promise = (async () => {
const { breakpoints } = await connection.sendBreakpointListCommand()
await Promise.all(
breakpoints
.filter(breakpoint => breakpoint.type === 'call')
.map(breakpoint => breakpoint.remove())
)
await Promise.all(
args.breakpoints.map(async (functionBreakpoint, index) => {
try {
await connection.sendBreakpointSetCommand(
new xdebug.CallBreakpoint(
functionBreakpoint.name,
functionBreakpoint.condition
)
)
vscodeBreakpoints[index] = { verified: true }
}
} catch (error) {
// only capture each breakpoint once
if (connectionIndex === 0) {
} catch (error) {
vscodeBreakpoints[index] = {
verified: false,
message: error instanceof Error ? error.message : error,
}
}
}
})
)
})
)
})()

if (connection.isPendingExecuteCommand) {
// There is a pending execute command which could lock the connection up, so do not
// wait on the response before continuing or it can get into a deadlock
promise.catch(err => this.sendEvent(new vscode.OutputEvent(util.inspect(err) + '\n')))
} else {
await promise
}
})
)
}
Expand Down
56 changes: 45 additions & 11 deletions src/xdebugConnection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,8 @@ interface Command {
resolveFn: (response: XMLDocument) => any
/** callback that gets called if an error happened while parsing the response */
rejectFn: (error?: Error) => any
/** whether command results in PHP code being executed or not */
isExecuteCommand: boolean
}

/**
Expand Down Expand Up @@ -585,6 +587,15 @@ export class Connection extends DbgpConnection {
*/
private _commandQueue: Command[] = []

private _pendingExecuteCommand = false
/**
* Whether a command was started that executes PHP, which means the connection will be blocked from
* running any additional commands until the execution gets to the next stopping point or exits.
*/
public get isPendingExecuteCommand(): boolean {
return this._pendingExecuteCommand
}

/** Constructs a new connection that uses the given socket to communicate with XDebug. */
constructor(socket: net.Socket) {
super(socket)
Expand All @@ -602,6 +613,7 @@ export class Connection extends DbgpConnection {
if (this._pendingCommands.has(transactionId)) {
const command = this._pendingCommands.get(transactionId)!
this._pendingCommands.delete(transactionId)
this._pendingExecuteCommand = false
command.resolveFn(response)
}
if (this._commandQueue.length > 0) {
Expand All @@ -624,15 +636,31 @@ export class Connection extends DbgpConnection {
*/
private _enqueueCommand(name: string, args?: string, data?: string): Promise<XMLDocument> {
return new Promise((resolveFn, rejectFn) => {
const command = { name, args, data, resolveFn, rejectFn }
if (this._commandQueue.length === 0 && this._pendingCommands.size === 0) {
this._executeCommand(command)
} else {
this._commandQueue.push(command)
}
this._enqueue({ name, args, data, resolveFn, rejectFn, isExecuteCommand: false })
})
}

/**
* Pushes a new execute command (one that results in executing PHP code) to the queue that will be executed after all the previous
* commands have finished and we received a response.
* If the queue is empty AND there are no pending transactions (meaning we already received a response and XDebug is waiting for
* commands) the command will be executed immediately.
*/
private _enqueueExecuteCommand(name: string, args?: string, data?: string): Promise<XMLDocument> {
return new Promise((resolveFn, rejectFn) => {
this._enqueue({ name, args, data, resolveFn, rejectFn, isExecuteCommand: true })
})
}

/** Adds the given command to the queue, or executes immediately if no commands are currently being processed. */
private _enqueue(command: Command): void {
if (this._commandQueue.length === 0 && this._pendingCommands.size === 0) {
this._executeCommand(command)
} else {
this._commandQueue.push(command)
}
}

/**
* Sends a command to XDebug with a new transaction ID and calls the callback on the command. This can
* only be called when XDebug can actually accept commands, which is after we received a response for the
Expand All @@ -649,8 +677,14 @@ export class Connection extends DbgpConnection {
}
commandString += '\0'
const data = iconv.encode(commandString, ENCODING)
await this.write(data)
this._pendingCommands.set(transactionId, command)
this._pendingExecuteCommand = command.isExecuteCommand
if (this._pendingExecuteCommand) {
// Since PHP execution commands block anything on the connection until it is
// done executing, emit that the connection is about to go into such a locked state
this.emit('before-execute-command')
}
await this.write(data)
}

public close() {
Expand Down Expand Up @@ -752,22 +786,22 @@ export class Connection extends DbgpConnection {

/** sends a run command */
public async sendRunCommand(): Promise<StatusResponse> {
return new StatusResponse(await this._enqueueCommand('run'), this)
return new StatusResponse(await this._enqueueExecuteCommand('run'), this)
}

/** sends a step_into command */
public async sendStepIntoCommand(): Promise<StatusResponse> {
return new StatusResponse(await this._enqueueCommand('step_into'), this)
return new StatusResponse(await this._enqueueExecuteCommand('step_into'), this)
}

/** sends a step_over command */
public async sendStepOverCommand(): Promise<StatusResponse> {
return new StatusResponse(await this._enqueueCommand('step_over'), this)
return new StatusResponse(await this._enqueueExecuteCommand('step_over'), this)
}

/** sends a step_out command */
public async sendStepOutCommand(): Promise<StatusResponse> {
return new StatusResponse(await this._enqueueCommand('step_out'), this)
return new StatusResponse(await this._enqueueExecuteCommand('step_out'), this)
}

/** sends a stop command */
Expand Down

0 comments on commit 7fa2d28

Please sign in to comment.