Avoid deadlock with multiple connections when executing code and new connection opens by jonyo · Pull Request #294 · xdebug/vscode-php-debug

jonyo · 2018-08-15T04:51:18Z

Fixes #164

Original PR: #292

This PR reworks how the original one does things after feedback and as noted below. It made it a lot cleaner to start fresh on a new branch which is why I didn't add the changes on to the original PR...

This introduces a new concept in the xDebug connection: "execution commands": these are commands that result in PHP being executed. The way xDebug works, if one of these commands is run, that connection is hung until the PHP code is done executing (until it gets to the next stopping point). By keeping track of when such commands are still in progress, and making it something that the adapter can check, it allows the adapter to specifically code for when the connection is currently hung. Which allows preventing the deadlock situation described in #164

The adapter also now emits before-execution-command since it results in possibly going into a hung state for a while. The adapter uses that event to send a ContinueEvent, this is just a side advantage that makes the interface more responsive. Before when you click continue, if the PHP code is hung for whatever reason (it is slow, it has a lot to do, etc), it appears that it did not work as it appears to still be stopped on a breakpoint (but it is not, which you can tell by trying to view any of the details in the debug pane). With the change, now when you click continue it immediately changes the process back to "running" state which is more truthful.

EDIT: Since #164 has a lot of comments, here is the tl;dr: deadlock explanation:

Connection 1 makes a second connection with curl.
It sends command to all connections to set breakpoints, and waits for response.
The second connection hangs as it is waiting for connection 1 to set breakpoints, connection 1 hangs without setting those breakpoints because it is waiting for connection 2 to respond and can't do anything until that completes.

This PR makes it not wait for the breakpoints to be set on a connection if the connection is waiting for code to execute. So now connection 2 can finish initializing and run, no more deadlock.

…ons when executing code

felixfbecker · 2018-08-15T09:56:45Z

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


Why is this optional?

😨 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.

felixfbecker · 2018-08-15T09:59:54Z

                    const command = this._pendingCommands.get(transactionId)!
                    this._pendingCommands.delete(transactionId)
                    command.resolveFn(response)
+                    this._pendingExecuteCommand = false;


Shouldn't this be set to false before resolving?

felixfbecker · 2018-08-15T10:00:50Z

        try {
            const fileUri = convertClientPathToDebugger(args.source.path!, this._args.pathMappings)
            const connections = Array.from(this._connections.values())
+            const hasWaitingConnections = this._waitingConnections.size > 0


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

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.

felixfbecker · 2018-08-15T10:02:38Z

+                            // 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;


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).

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.

…mmand

…e response

…ction is currently executing

jonyo · 2018-08-15T17:01:21Z

@felixfbecker Changes in, thanks!

codecov · 2018-08-15T17:25:12Z

Codecov Report

Merging #294 into master will increase coverage by 0.83%.
The diff coverage is 81.35%.

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   69.34%   70.17%   +0.83%     
==========================================
  Files           5        5              
  Lines         995     1016      +21     
  Branches      161      161              
==========================================
+ Hits          690      713      +23     
+ Misses        305      303       -2

Impacted Files	Coverage Δ
src/phpDebug.ts	`66.93% <76.92%> (+0.48%)`	⬆️
src/xdebugConnection.ts	`84.64% <90%> (+1.58%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08a5500...31948fa. Read the comment docs.

…Breakpoints

… to avoid waiting

Mancy · 2018-08-16T20:51:00Z

Thank you Jonyo, you have saved my time with this fix

macbookandrew · 2018-08-27T16:58:29Z

Hate to ask a noob question, but how can I use this change on my machine?

felixfbecker · 2018-08-28T07:27:22Z

-                                try {
-                                    await connection.sendBreakpointSetCommand(breakpoint)
-                                    // only capture each breakpoint once
-                                    if (connectionIndex === 0) {


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

@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.

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.

felixfbecker · 2018-08-28T13:43:01Z

🎉 This PR is included in version 1.12.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

…ebug#294) This introduces a new concept in the xDebug connection: "execution commands": these are commands that result in PHP being executed. The way xDebug works, if one of these commands is run, that connection is hung until the PHP code is done executing (until it gets to the next stopping point). By keeping track of when such commands are still in progress, and making it something that the adapter can check, it allows the adapter to specifically code for when the connection is currently hung. Which allows preventing the deadlock situation described in xdebug#164 The adapter also now emits `before-execution-command` since it results in possibly going into a hung state for a while. The adapter uses that event to send a `ContinueEvent`, this is just a side advantage that makes the interface more responsive. Before when you click continue, if the PHP code is hung for whatever reason (it is slow, it has a lot to do, etc), it appears that it did not work as it appears to still be stopped on a breakpoint (but it is not, which you can tell by trying to view any of the details in the debug pane). With the change, now when you click continue it immediately changes the process back to "running" state which is more truthful. Since xdebug#164 has a lot of comments, here is the **tl;dr:** deadlock explanation: 1. Connection 1 makes a second connection with curl. 2. It sends command to all connections to set breakpoints, and waits for response. 3. The second connection hangs as it is waiting for connection 1 to set breakpoints, connection 1 hangs without setting those breakpoints because it is waiting for connection 2 to respond and can't do anything until that completes. This PR makes it not wait for the breakpoints to be set on a connection if the connection is waiting for code to execute. So now connection 2 can finish initializing and run, no more deadlock. Fixes xdebug#164

fix(phpDebug,xdebugConnection): Avoid deadlock with multiple connecti…

fe5855b

…ons when executing code

felixfbecker reviewed Aug 15, 2018

View reviewed changes

jonyo added 4 commits August 15, 2018 08:11

fix(xdebugConnection): isExecuteCommand does not need to be optional

17c8ea0

docs(xdebugConnection): Adding jsdoc for Connection._pendingExecuteCo…

064abdc

…mmand

fix(xdebugConnection): set _pendingExecuteCommand before resolving th…

48ca0f4

…e response

refactor(phpDebug): Do not wait when setting breakpoints if the conne…

e8cadff

…ction is currently executing

style(phpDebug): Applying pretty code changes

5bbc91b

Merge branch 'master' into deadlock

35612ca

jonyo mentioned this pull request Aug 16, 2018

Prevent deadlock by only updating breakpoints on needed connections #292

Closed

jonyo changed the title ~~Avoid deadlock with multiple connections when executing code and new connection opens~~ WIP Avoid deadlock with multiple connections when executing code and new connection opens Aug 16, 2018

jonyo added 3 commits August 16, 2018 09:26

fix(phpDebug): The first index could be skipped, so always set vscode…

97c5db9

…Breakpoints

refactor(phpDebug): No need to chain all promises, just the first one…

dcbeebc

… to avoid waiting

docs(phpDebug): Adding back inline comments and renaming a var

d145cb9

jonyo changed the title ~~WIP Avoid deadlock with multiple connections when executing code and new connection opens~~ Avoid deadlock with multiple connections when executing code and new connection opens Aug 16, 2018

felixfbecker added 2 commits August 27, 2018 19:30

refactor: improve error handling and simplify

7d80911

refactor: use async/await again

31948fa

felixfbecker reviewed Aug 28, 2018

View reviewed changes

felixfbecker approved these changes Aug 28, 2018

View reviewed changes

felixfbecker merged commit 7fa2d28 into xdebug:master Aug 28, 2018

ettoredn mentioned this pull request Sep 4, 2018

Debug context lost on multiple connections #301

Closed

Uh oh!

Uh oh!

Conversation

jonyo commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonyo commented Aug 15, 2018

Uh oh!

codecov Bot commented Aug 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mancy commented Aug 16, 2018

Uh oh!

macbookandrew commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonyo commented Aug 15, 2018 •

edited

Loading

codecov Bot commented Aug 15, 2018 •

edited by ghost

Loading