Refactor continue events to avoid UI hanging #308
Conversation
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
==========================================
+ Coverage 70.05% 71.27% +1.21%
==========================================
Files 5 5
Lines 1012 992 -20
Branches 161 157 -4
==========================================
- Hits 709 707 -2
+ Misses 303 285 -18
Continue to review full report at Codecov.
|
src/phpDebug.ts
Outdated
| @@ -279,10 +279,6 @@ class PhpDebugSession extends vscode.DebugSession { | |||
| }) | |||
| connection.on('error', disposeConnection) | |||
| connection.on('close', disposeConnection) | |||
| connection.on('before-execute-command', () => { | |||
There was a problem hiding this comment.
Is that event still used at all?
| this.sendResponse(response) | ||
| this._checkStatus(xdebugResponse) | ||
| this._checkStatus(await connection.sendRunCommand()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- success, now it is paused again - we let xdebug know in
_checkStatus() - Some exception / error like fatal error - this is also sort of success, the code was run and it is now paused on some problem in the code, so we let vscode know about it in
_checkStatus() - The script finishes. We let vscode know about that on https://github.com/felixfbecker/vscode-php-debug/blob/cf7ad01339f5ea053cc5117f101d6db085112492/src/phpDebug.ts#L281
- There is some connection level error, we let vscode know about those here:
https://github.com/felixfbecker/vscode-php-debug/blob/cf7ad01339f5ea053cc5117f101d6db085112492/src/phpDebug.ts#L277-L280 - There is a server level error, we let vscode know about those here: https://github.com/felixfbecker/vscode-php-debug/blob/cf7ad01339f5ea053cc5117f101d6db085112492/src/phpDebug.ts#L309-L312
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/phpDebug.ts
Outdated
| @@ -1008,7 +969,6 @@ class PhpDebugSession extends vscode.DebugSession { | |||
| this.sendErrorResponse(response, error) | |||
There was a problem hiding this comment.
This line is not valid anymore because the response has already been sent.
There was a problem hiding this comment.
Hmm, good point. I'll change that part.
There was a problem hiding this comment.
@felixfbecker I ended up reverting this part to how it was. Really the docs are rather vague so we are forced to figure out whether we should send immediately or wait. My original thought was to not wait since the response has no information to tell vscode, it is just an acknowledgment.
But now I'm thinking, this is a disconnect response, there is no harm in waiting, especially since it already has timeouts built in.
In addition, this could be a part of a restart request, in which case we do definitely want to wait before telling vscode, I could see a situation where we let vscode know too early and it results in vscode attempting to start a new connection before the old one is done. You would end up with errors about the port already being in use.
tl;dr: No harm in making it work like it did before, and there is potentially a problem by not waiting, so better to be on the safe side since we don't have much direction from the docs.
A debug adapter is not expected to send this event in response to a request that implies that execution continues, e.g. 'launch' or 'continue'. It is only necessary to send a 'continued' event if there was no previous request that implied this. Note: there is still the problem that it will seem to "hang" when you continue and the PHP code takes a while to run, #308 will fix that. Fixes #301
jonyo
force-pushed
the
refactor-continue-events
branch
from
cf7ad01
to
0e4ddb4
Compare
A debug adapter is not expected to send this event in response to a request that implies that execution continues, e.g. 'launch' or 'continue'. It is only necessary to send a 'continued' event if there was no previous request that implied this. Note: there is still the problem that it will seem to "hang" when you continue and the PHP code takes a while to run, xdebug#308 will fix that. Fixes xdebug#301
|
Already solved by #367. |
If you click next or continue and the PHP code is taking a while to run, it seems like the button did not have an effect because the context still shows and it still appears to be paused. The only hint that it worked, is that if you try to interact with the context, or eval any code in the console, it won't work because it has actually continued.
Looking at the docs for the continue, step into, etc. events, they indicate that it should actually send the response right away indicating that the action has started. Then after the action is complete, send the appropriate stop event depending on if it is now stopped on breakpoint etc.
But the way it was before, it did not do this, it waited until the next pause point before sending both the initial response and the one run after it is done. This was causing the UI to hang as described above.
Here is the applicable reference documentation I relied on when making this change:
https://github.com/Microsoft/vscode-debugadapter-node/blob/750f50bc875669fe3cb7594804bcfc6326b27d0e/protocol/src/debugProtocol.ts#L559
(my emphasis added)
Relates to #307 #301