Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid deadlock with multiple connections when executing code and new connection opens #294

Merged
merged 12 commits into from
Aug 28, 2018
Merged

Avoid deadlock with multiple connections when executing code and new connection opens #294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fe5855b
fix(phpDebug,xdebugConnection): Avoid deadlock with multiple connecti…
jonyo Aug 15, 2018
17c8ea0
fix(xdebugConnection): isExecuteCommand does not need to be optional
jonyo Aug 15, 2018
064abdc
docs(xdebugConnection): Adding jsdoc for Connection._pendingExecuteCo…
jonyo Aug 15, 2018
48ca0f4
fix(xdebugConnection): set _pendingExecuteCommand before resolving th…
jonyo Aug 15, 2018
e8cadff
refactor(phpDebug): Do not wait when setting breakpoints if the conne…
jonyo Aug 15, 2018
5bbc91b
style(phpDebug): Applying pretty code changes
jonyo Aug 15, 2018
35612ca
Merge branch 'master' into deadlock
jonyo Aug 16, 2018
97c5db9
fix(phpDebug): The first index could be skipped, so always set vscode…
jonyo Aug 16, 2018
dcbeebc
refactor(phpDebug): No need to chain all promises, just the first one…
jonyo Aug 16, 2018
d145cb9
docs(phpDebug): Adding back inline comments and renaming a var
jonyo Aug 16, 2018
7d80911
refactor: improve error handling and simplify
felixfbecker Aug 27, 2018
31948fa
refactor: use async/await again
felixfbecker Aug 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/phpDebug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -438,6 +442,7 @@ class PhpDebugSession extends vscode.DebugSession {
try {
const fileUri = convertClientPathToDebugger(args.source.path!, this._args.pathMappings)
const connections = Array.from(this._connections.values())
const hasWaitingConnections = this._waitingConnections.size > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me why the existence of a waiting connection is of significance for this behavior. Could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well as you said, the way I'm doing it now, just skipping the connection altogether if it is blocked, is rather brittle. So my thought was, the deadlock specifically happens because the configuration of a new connection is waiting on the configuration to be initialized, which involves calling this code to set up breakpoints. So if _waitingConnections has something in it, that is when we should "care" that there could potentially be a command blocking it from continuing.

That said, if I change it's behavior to instead do a delayed update instead of just skipping the connection, this would no longer be needed. So if I get that working I'll remove this condition as well.

let xdebugBreakpoints: Array<xdebug.ConditionalBreakpoint | xdebug.LineBreakpoint>
response.body = { breakpoints: [] }
// this is returned to VS Code
Expand All @@ -458,6 +463,12 @@ class PhpDebugSession extends vscode.DebugSession {
// for all connections
await Promise.all(
connections.map(async (connection, connectionIndex) => {
if (hasWaitingConnections && connection.isPendingExecuteCommand()) {
// skip this if there is a connection that is being initialized, and this
// connection is in middle of running PHP code. This avoids a deadlock that
// can happen if the PHP code initializes a new connection that uses xdebug
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply ignoring this setBreakpoint command seems brittle. The way I would imagine it is to still send the command (queue it) but don't wait for the response (and send a potential error to the debug console).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see if I can get it to work like that. I was hesitant to try that way as I think it would involve the exact same code (so a lot of duplicated code), just without the await, instead maybe using .then().

I'll try it and see if it works like that.

}
// 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()
Expand Down Expand Up @@ -511,8 +522,15 @@ class PhpDebugSession extends vscode.DebugSession {
) {
try {
const connections = Array.from(this._connections.values())
const hasWaitingConnections = this._waitingConnections.size > 0
await Promise.all(
connections.map(async connection => {
if (hasWaitingConnections && connection.isPendingExecuteCommand()) {
// skip this if there is a connection that is being initialized, and this
// connection is in middle of running PHP code. This avoids a deadlock that
// can happen if the PHP code initializes a new connection that uses xdebug
return;
}
// get all breakpoints
const { breakpoints } = await connection.sendBreakpointListCommand()
// remove all exception breakpoints
Expand Down Expand Up @@ -542,6 +560,7 @@ class PhpDebugSession extends vscode.DebugSession {
) {
try {
const connections = Array.from(this._connections.values())
const hasWaitingConnections = this._waitingConnections.size > 0
// this is returned to VS Code
let vscodeBreakpoints: VSCodeDebugProtocol.Breakpoint[]
if (connections.length === 0) {
Expand All @@ -552,6 +571,12 @@ class PhpDebugSession extends vscode.DebugSession {
// for all connections
await Promise.all(
connections.map(async (connection, connectionIndex) => {
if (hasWaitingConnections && connection.isPendingExecuteCommand()) {
// skip this if there is a connection that is being initialized, and this
// connection is in middle of running PHP code. This avoids a deadlock that
// can happen if the PHP code initializes a new connection that uses xdebug
return;
}
// clear breakpoints for this file
const { breakpoints } = await connection.sendBreakpointListCommand()
await Promise.all(
Expand Down
57 changes: 46 additions & 11 deletions src/xdebugConnection.ts
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨 that was an artifact of how I started out implementing, I started making it an optional parameter on the main _enqueueCommand method. It no longer needs to be optional, I'll change it.

}

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

private _pendingExecuteCommand = false

/** Constructs a new connection that uses the given socket to communicate with XDebug. */
constructor(socket: net.Socket) {
super(socket)
Expand All @@ -603,6 +607,7 @@ export class Connection extends DbgpConnection {
const command = this._pendingCommands.get(transactionId)!
this._pendingCommands.delete(transactionId)
command.resolveFn(response)
this._pendingExecuteCommand = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be set to false before resolving?

}
if (this._commandQueue.length > 0) {
const command = this._commandQueue.shift()!
Expand All @@ -617,22 +622,46 @@ export class Connection extends DbgpConnection {
return this._initPromise
}

/**
* 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 isPendingExecuteCommand(): boolean {
return this._pendingExecuteCommand;
}

/**
* Pushes a new command 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 _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 +678,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 +787,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