Skip to content

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

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

Merged
felixfbecker 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 all commits
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
149 changes: 83 additions & 66 deletions src/phpDebug.ts
View file
Open in desktop
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) {

@felixfbecker felixfbecker Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I simplified the code and noticed that you removed these if checks. Was that intentional?

@jonyo jonyo Aug 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker Yes, because connectionIndex of 0 could be the connection that is waiting for PHP code to execute, connectionIndex 1 would be the "new" connection that is able to execute right away so is able to set the value in time for it to matter.

Plus all it is doing is setting a value and the value is fixed, it doesn't save time by skipping it if already set.

@felixfbecker felixfbecker Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I understand. There is a slight behaviour change here in that now if any of the connections errored, the breakpoint will be marked us unverified, whereas before, only the first connection determined the fate of the breakpoint. This is fine I guess.

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
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
}

/**
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