Skip to content

Refactor continue events to avoid UI hanging #308

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

Closed
jonyo wants to merge 2 commits into from
Closed

Refactor continue events to avoid UI hanging #308

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e4ddb4
refactor(phpDebug): Send response immediately for continue commands, …
jonyo Sep 19, 2018
8bbc53f
revert(phpDebug): wait for connections to be done before sending resp…
jonyo Sep 21, 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
72 changes: 18 additions & 54 deletions src/phpDebug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,88 +889,52 @@ class PhpDebugSession extends vscode.DebugSession {
response: VSCodeDebugProtocol.ContinueResponse,
args: VSCodeDebugProtocol.ContinueArguments
) {
let xdebugResponse: xdebug.StatusResponse | undefined
try {
const connection = this._connections.get(args.threadId)
if (!connection) {
throw new Error('Unknown thread ID ' + args.threadId)
}
xdebugResponse = await connection.sendRunCommand()
} catch (error) {
this.sendErrorResponse(response, error)
if (xdebugResponse) {
this._checkStatus(xdebugResponse)
}
return
}
response.body = {
allThreadsContinued: false,
}
const connection = this._connections.get(args.threadId)
if (!connection) {
this.sendErrorResponse(response, new Error('Unkown thread ID ' + args.threadId))
return
}
this.sendResponse(response)
this._checkStatus(xdebugResponse)
this._checkStatus(await connection.sendRunCommand())

@felixfbecker felixfbecker Sep 21, 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.

The problem with this is that you are not handling the error anymore. The response to this command is the only chance to report that the run command failed. XDebug doesn't return a response to the continue command unless the command failed or the program stopped again.

If the only way is to send the response straight away, it needs to at least log errors, or only send the response after a timeout.

@jonyo jonyo Sep 21, 2018 •
edited
Loading

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

If the only way is to send the response straight away, it needs to at least log errors, or only send the response after a timeout.

How I interpreted the docs, I believe that we do need to send the response right away on requests that are running more code as an acknowledgment that it has started. As I understand it (again the docs are vague), it should use sendErrorResponse() only if there is some reason it cannot start the action. The results of the action are sent up the line to vscode in other ways already, these are the different outcomes I can think of:

It looked like the try/catch was specifically for handling when the connection was not found with that threadId, and when that is the case, we now handle it directly by responding with sendErrorResponse(). As far as I could tell it would never throw an exception as part of the call to connection.sendRunCommand(), instead it would either return a response with an error in it (handled by _checkStatus(), or emit an event which the phpDebug is already handling on it's own as noted above.

All vscode needs to know here is that we started running the command, after that point if there is an error we let vscode know about it in other emitted events.

@felixfbecker felixfbecker Nov 16, 2019

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.

Sorry for the late reply. I agree with what you say, but that doesn't change the fact that sendRunCommand() can reject with an exception, and that exception needs to be handled somehow. Even if it is handled async and we send the response before, it needs an error handler that at a minimum needs to send an error log to VS Code.

@jonyo jonyo Nov 16, 2019

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.

Ah, I see what you mean, if the xdebug socket is not writable it will reject so need to catch that.

If you are in agreement about using async calls (with addition of handling the reject error) let me know. If interested I'd be happy to rebase (if possible after a year) or just redo this PR when I have some time.

@felixfbecker felixfbecker Nov 16, 2019

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.

Yes that sounds good

}

protected async nextRequest(response: VSCodeDebugProtocol.NextResponse, args: VSCodeDebugProtocol.NextArguments) {
let xdebugResponse: xdebug.StatusResponse | undefined
try {
const connection = this._connections.get(args.threadId)
if (!connection) {
throw new Error('Unknown thread ID ' + args.threadId)
}
xdebugResponse = await connection.sendStepOverCommand()
} catch (error) {
this.sendErrorResponse(response, error)
if (xdebugResponse) {
this._checkStatus(xdebugResponse)
}
const connection = this._connections.get(args.threadId)
if (!connection) {
this.sendErrorResponse(response, new Error('Unkown thread ID ' + args.threadId))
return
}
this.sendResponse(response)
this._checkStatus(xdebugResponse)
this._checkStatus(await connection.sendStepOverCommand())
}

protected async stepInRequest(
response: VSCodeDebugProtocol.StepInResponse,
args: VSCodeDebugProtocol.StepInArguments
) {
let xdebugResponse: xdebug.StatusResponse | undefined
try {
const connection = this._connections.get(args.threadId)
if (!connection) {
throw new Error('Unknown thread ID ' + args.threadId)
}
xdebugResponse = await connection.sendStepIntoCommand()
} catch (error) {
this.sendErrorResponse(response, error)
if (xdebugResponse) {
this._checkStatus(xdebugResponse)
}
const connection = this._connections.get(args.threadId)
if (!connection) {
this.sendErrorResponse(response, new Error('Unkown thread ID ' + args.threadId))
return
}
this.sendResponse(response)
this._checkStatus(xdebugResponse)
this._checkStatus(await connection.sendStepIntoCommand())
}

protected async stepOutRequest(
response: VSCodeDebugProtocol.StepOutResponse,
args: VSCodeDebugProtocol.StepOutArguments
) {
let xdebugResponse: xdebug.StatusResponse | undefined
try {
const connection = this._connections.get(args.threadId)
if (!connection) {
throw new Error('Unknown thread ID ' + args.threadId)
}
xdebugResponse = await connection.sendStepOutCommand()
} catch (error) {
this.sendErrorResponse(response, error)
if (xdebugResponse) {
this._checkStatus(xdebugResponse)
}
const connection = this._connections.get(args.threadId)
if (!connection) {
this.sendErrorResponse(response, new Error('Unkown thread ID ' + args.threadId))
return
}
this.sendResponse(response)
this._checkStatus(xdebugResponse)
this._checkStatus(await connection.sendStepOutCommand())
}

protected pauseRequest(response: VSCodeDebugProtocol.PauseResponse, args: VSCodeDebugProtocol.PauseArguments) {
Expand Down