Prevent deadlock by only updating breakpoints on needed connections #292
Conversation
|
@felixfbecker Are tests currently broken? I tried running Looks like it is failing due to security warnings on the npm dependencies (which this PR doesn't touch). |
|
They pass on master: What does |
$ npm -v
4.2.0
$ node -v
v7.9.0
$ php -v
PHP 7.1.16 (cli) (built: Mar 31 2018 02:59:59) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
with Xdebug v2.5.5, Copyright (c) 2002-2017, by Derick Rethans |
| const data = iconv.encode(commandString, ENCODING) | ||
| await this.write(data) | ||
| this._pendingCommands.set(transactionId, command) | ||
| await this.write(data) |
|
|
||
| /** sends a run command */ | ||
| public async sendRunCommand(): Promise<StatusResponse> { | ||
| this.emit('continue-event'); |
There was a problem hiding this comment.
"ContinueEvent" is a VS Code concept, but this class should have no knowledge of VS Code, only of XDebug. I would prefer if callers of these send*Command() functions in PhpDebugSession sent the ContinuedEvent to VS Code, without this layer of event indirection.
There was a problem hiding this comment.
@felixfbecker I agree, at the very least it should not emit something that only means something to vscode.
So, when you make any command that executes 1 or more lines of PHP code, the xdebug connection will not respond until the code is done executing, it is essentially locked up. Given that, maybe it does make sense to have the logic in xdebugConnection? Just change the name to be more appropriate like code-execution-started. Instead of emmitting when vscode status should change to ContinuedEvent it is just emmitting that some command was called that will result in PHP code being executed.
I'm fine with doing it either way, changing the event name OR changing the caller to emit the ContinuedEvent.
Also if you think this requires more discussion I can move that change to it's own PR? Since it is not required for the actual deadlock fix, it's more of a UX improvement, I can break it out if you want.
| const fileUri = convertClientPathToDebugger(args.source.path!, this._args.pathMappings) | ||
| const connections = Array.from(this._connections.values()) | ||
| // If there is a waiting connection, only use use that as it is likely being called to init | ||
| // the breakpoints on the new connection |
There was a problem hiding this comment.
What concerns me about this fix is the word "likely". "likely" to me means "race condition". Can this be fixed without having to rely on the likelihood of something?
I think fundamentally the problem here is that for a single VS Code <-> debug adapter connection we need to persist the breakpoints to multiple connected PHP processes. Some may block because they are mid-execution, some may succeed, some may fail, but we only have a single request from VS Code to handle and respond to (with verified or error).
@weinand any advice how to handle this scenario best?
There was a problem hiding this comment.
@felixfbecker Yeah, a more stable fix would be to somehow account for success, failed, or blocked and the blocked would not stop configurationDone from firing. Ideally if there was some concrete way to tell that the breakpoint request was part of the process of initializing a new connection, I think it is still a good idea to only run it on the new connection in that case.
The tricky part here is that as far as I can tell these are initiated from vscode itself. I used the word likely because of that, since _pendingConnections is not empty that means a new connection was just added. For it to "not" be because of the new connection, the user would have had to do something like change a breakpoint at the precise moment a new connection was established, which is always a possibility.
Maybe we first: when doing something that executes code, set a flag like _isLocked that gets un-set when the response resolves. Then when making the request to do something with breakpoints, if connection._isLocked it would queue the command but if there are more than one connection, as long as one succeeds it would continue. That would allow the after configuration stuff to run for the new connection.
I hope that makes sense?
There was a problem hiding this comment.
In other words, instead of Promise.All, use something like Promise.race, so it continues after at least one is done...
|
Try updating npm to latest (6.3.0). If it still doesn't work, post me the error |
|
@felixfbecker What version of node should I be using for this? After updating npm it warns about using npm with this version of node, will it work with latest version of node? |
|
It used to be 7.9, but VS Code just updated to 8.9.3 #293 |
jonyo
force-pushed
the
secondary-request-deadlock
branch
from
0ecbc1b to
df59334
Compare
|
@felixfbecker Still getting errors even on master on my local. I noticed there is now php_errors.log I wonder if it is something to do with php.ini settings? PHP Debug Adapter
initialization
✓ should return supported features
launch as CLI
✓ should error on non-existing file
1) should run program to the end
2) should stop on entry
✓ should not stop if launched without debugging
continuation commands
- should handle run
- should handle step_over
- should handle step_in
- should handle step_out
✓ should error on pause request
3) should handle disconnect
breakpoints
line breakpoints
4) should stop on a breakpoint
5) should stop on a breakpoint in file with spaces in its name
6) should stop on a breakpoint identical to the entrypoint
7) should support removing a breakpoint
exception breakpoints
8) should not break on anything if the file matches the ignore pattern
9) should support stopping only on a notice
10) should support stopping only on a warning
- should support stopping only on an error
11) should support stopping only on an exception
12) should support stopping on everything
- should report the error in a virtual error scope
conditional breakpoints
13) should stop on a conditional breakpoint when condition is true
14) should not stop on a conditional breakpoint when condition is false
function breakpoints
15) should stop on a function breakpoint
variables
16) "before each" hook for "should report scopes correctly"
virtual sources
- should break on an exception inside eval code
- should return the eval code with a source request
parallel requests
- should report multiple requests as threads
evaluation
- should return the eval result
- should return variable references for structured results
output events
- stdout and stderr events should be complete and in correct order
DbgpConnection
✓ should parse a response in one data event
✓ should parse a response over multiple data events
✓ should parse multiple responses in one data event
✓ should error on invalid XML
paths
isSameUri
✓ should compare to URIs
✓ should compare windows paths case-insensitive
convertClientPathToDebugger
without source mapping
✓ should convert a windows path to a URI
✓ should convert a unix path to a URI
with source mapping
✓ should convert a unix path to a unix URI
✓ should convert a unix path to a windows URI
- should convert a windows path to a unix URI
- should convert a windows path with inconsistent casing to a unix URI
- should convert a windows path to a windows URI
convertDebuggerPathToClient
without source mapping
- should convert a windows URI to a windows path
✓ should convert a unix URI to a unix path
- should handle non-unicode special characters
with source mapping
✓ should map unix uris to unix paths
- should map unix uris to windows paths
✓ should map windows uris to unix paths
- should map windows uris to windows paths
17 passing (13m)
19 pending
16 failing
1) PHP Debug Adapter
launch as CLI
should run program to the end:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
2) PHP Debug Adapter
launch as CLI
should stop on entry:
Error: no event 'stopped' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
3) PHP Debug Adapter
continuation commands
should handle disconnect:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
4) PHP Debug Adapter
breakpoints
line breakpoints
should stop on a breakpoint:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
5) PHP Debug Adapter
breakpoints
line breakpoints
should stop on a breakpoint in file with spaces in its name:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
6) PHP Debug Adapter
breakpoints
line breakpoints
should stop on a breakpoint identical to the entrypoint:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
7) PHP Debug Adapter
breakpoints
line breakpoints
should support removing a breakpoint:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
8) PHP Debug Adapter
breakpoints
exception breakpoints
should not break on anything if the file matches the ignore pattern:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
9) PHP Debug Adapter
breakpoints
exception breakpoints
should support stopping only on a notice:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
10) PHP Debug Adapter
breakpoints
exception breakpoints
should support stopping only on a warning:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
11) PHP Debug Adapter
breakpoints
exception breakpoints
should support stopping only on an exception:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
12) PHP Debug Adapter
breakpoints
exception breakpoints
should support stopping on everything:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
13) PHP Debug Adapter
breakpoints
conditional breakpoints
should stop on a conditional breakpoint when condition is true:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
14) PHP Debug Adapter
breakpoints
conditional breakpoints
should not stop on a conditional breakpoint when condition is false:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
15) PHP Debug Adapter
breakpoints
function breakpoints
should stop on a function breakpoint:
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
16) PHP Debug Adapter
variables
"before each" hook for "should report scopes correctly":
Error: no event 'initialized' received after 10000 ms
at Timeout.setTimeout [as _onTimeout] (node_modules/vscode-debugadapter-testsupport/lib/debugClient.js:222:28)
npm ERR! Test failed. See above for more details. |
|
Is XDebug enabled? What is |
|
@felixfbecker yes xdebug seems to be enabled. Here is the output of |
|
You need to configure XDebug as described in the README, for example |
|
Closing to restructure how it avoids the deadlock to be more stable per comments. |
|
@felixfbecker you asked:
Sorry, but from the information in this issue I couldn't figure out what the scenario is. |
|
@weinand By the way, here is the new PR: #294 I made a new PR since it ended up being almost complete re-write, though in hindsight I should have just kept the original PR to make discussion easier. Anyways, to answer what the scenario is:
This is potentially something that at the very least, causes the extension to hang, in other situations as well. So in #294 I address that by introducing "execution commands", so that if the connection has issued a command resulting in PHP code being executed, the extension knows not to wait around for the response because it depends on how long PHP happens to take to get to the next stop point. And in the above scenario it causes a deadlock (at least until curl times out). |
3 participants
This fixes #164
In this PR:
_waitingConnections, it only uses that connection instead of all. When that is the case the breakpoints are likely only getting updated as part of initializing the new connection. This fixes the main problem mentioned in Request making a second request makes it hang #164 that was caused because it was sending the breakpoint calls to the first connection, when that connection was locked up waiting on response from second connection. Doing it this way eliminates the deadlock, at least in this scenario._pendingConnectionsbefore it makes the call, not after. This ensures it correctly only attempts to send one command at a time and waits for the response before sending another.ContinuedEvent. With the change, now once you hit continue or step into etc. it immediately behaves like the execution has continued. Before the change if there was something like a sleep() command, when you hit continue, it would "seem" like nothing happened as it appears to still be paused on a breakpoint when that is not the case.