Page MenuHomePhabricator

[vscode] set default values for terminateDebuggee for the disconnect request
ClosedPublic

Authored by wallace on Jun 4 2020, 4:05 PM.

Details

Summary

Recently I've noticed that VSCode sometimes doesn't send the terminateDebuggee flag within the disconnectRequest,
even though lldb-vscode sets the terminateDebuggee capability correctly.
This has been causing that inferiors don't die after the debug session ends, and many users have reported issues because of this.

An easy way to mitigate this is to set better default values for the terminateDebuggee field in the disconnect request.
I'm assuming that for a launch request, the default will be true, and for attach it'll be false.

Diff Detail

Event Timeline

wallace created this revision.Jun 4 2020, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 4:05 PM
clayborg requested changes to this revision.Jun 4 2020, 6:17 PM

So the main question is why was lldb-vscode not exiting. Do we know why? Some thread wasn't exiting I would guess, but usually as long as the main thread exits, all other threads will just be terminated. Our main thread should be the loop that waits for packets, gets one, executes what it needs to and sends a response. When the "terminate" packet comes in, that read loop should exit and this should cause the lldb-vscode to exit. So I am questioning if this works around the issue. Does "disconnect" always get sent for both launch and attach? Then "terminate"?

The changes look good for other reasons, but I am wondering if this will fix all of the lldb-vscode binaries that we have sitting around. A quick "sample lldb-vscode" should show where these lldb-vscode binaries are stuck. Can we verify what is going on to make sure this will fix this?

Marking as needs changes due to initialization of the "VSCode::is_attach" not being done in constructor.

lldb/test/API/tools/lldb-vscode/disconnect/main.c
10 ↗(On Diff #268613)

This comment seems off? You can call "pause()" to wait for a signal, but that won't work on non unix based systems. Is the comment just left over?

lldb/tools/lldb-vscode/VSCode.h
92

This should be initialized in the VSCode::VSCode constructor? Surely we are initializing the other values right?

This revision now requires changes to proceed.Jun 4 2020, 6:17 PM
wallace marked 2 inline comments as done.Jun 4 2020, 6:39 PM

Well, the problem that i've seen happens mostly with long running processes like services that just don't die. So this fixes those issues anyway because those processes are not dying when they should. I tried an older version of the IDE and it sends all the time the terminateDebuggee flag.
Anyway, what you mention made me notice something. This is the existing flow without this change

vscode sends a disconnect request. At this point the UI might stop showing the debug session, but it's still running

< lldb-vscode receives it and doesn't terminate the inferior nor terminates itself
< the inferior keeps running and whenever there's output, it's sent to vscode
< if the process terminates, it sends a terminate event

vscode then finishes the debug session

A problem that I see here is that lldb-vscode was still alive. I'll investigate that tomorrow.

Anyway,wWe still need to investigate why some other processes don't die under normal circumstances, but I'll do it in a different patch.

lldb/test/API/tools/lldb-vscode/disconnect/main.c
10 ↗(On Diff #268613)

ah yes, it's a left over. I don't want to do something too posix dependant. Good catch

lldb/tools/lldb-vscode/VSCode.h
92

oh thank you, i didn't notice that constructor :)

labath added inline comments.Jun 5 2020, 1:57 AM
lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py
56

Writing an attach test is tricky because of the yama restrictions on linux. The way that we handle that right now is to have the inferior call lldb_enable_attach() and then create a token file. The test then waits for the token file lldbutil.wait_for_file_on_target and only then attaches. This ensures the attack takes place only when yama is disabled.

69

I'm confused as to why we're not expecting any output here. I was under the impression that we want to let the inferior continue in the "attach" case.

wallace updated this revision to Diff 270038.Wed, Jun 10, 11:21 PM

I updated the tests and did some minor changes.

@clayborg, after some testing, I noticed that this doesn't fix the problem of lldb-vscode instances not dying, however it solves the problem
of long-running inferiors not dying. Both are issues anyway.

I'll send another patch fixing the first problem, but at least this solves the second one.

Btw, I mentioned before that even after detaching, lldb-vscode was capturing some output by the inferior, however this is fine.
lldb-vscode captures just some of the inferior's output in its even loop until it gets the eBroadcastBitStopEventThread event,
after that lldb-vscode dies normally. So this diff should be fine.

clayborg added inline comments.Thu, Jun 11, 12:38 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
796

I am not sure what returning an error to disconnect will do other than hosing the IDE in some way. It really doesn't matter if we succeeded, we should probably just not mess with the return value. Thoughts?

wallace marked an inline comment as done.Thu, Jun 11, 3:05 PM
wallace added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
796

This is actually quite useful when analyzing logs, as we could know if any disconnection ended up in an internal failure.
This is safe, as I've tried returning a hardcoded error in this case and the IDE terminated the debug session gracefully anyway. The error field is accepted by the protocol, btw.

clayborg accepted this revision.Thu, Jun 11, 10:21 PM

I'm ok with this then if the error being returned for disconnect doesn't affect the IDE. Anyone else should chime in quickly if you have any objections.

This revision is now accepted and ready to land.Thu, Jun 11, 10:21 PM
labath added inline comments.Fri, Jun 12, 1:52 AM
lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py
69

When I think about this more, I get even more confused. In the attach case, is the inferior stdout even flowing through the vscode connection? I would expect not..

If that's true, then this test and its description is pretty misleading -- there will not be anything in the stdout stream, but not because we disconnected early -- the stdout will _never_ be there.

What exactly are you trying to test here? Maybe it would be more worthwhile to read the stdout from the popen object to ensure that the inferior does continue to print the number correctly?

wallace updated this revision to Diff 270804.EditedMon, Jun 15, 11:02 AM
wallace edited the summary of this revision. (Show Details)
wallace removed a reviewer: kusmour.

The tests were weird indeed, I think I had to revisit them after making some changes.
Anyway, I've updated the tests and they do make sense now. They test if the inferior performs some side effect after
it's disconnected.

wallace marked 4 inline comments as done.Mon, Jun 15, 1:43 PM

@labath , does this look better now?

wallace edited the summary of this revision. (Show Details)Tue, Jun 16, 3:07 PM
wallace updated this revision to Diff 271224.Tue, Jun 16, 3:19 PM
wallace edited the summary of this revision. (Show Details)

remove unwanted changes

labath accepted this revision.Mon, Jun 22, 4:03 AM

Yes, this looks better, though I am mildly worried about the use of timeouts.

There probably isn't anything better we can do for the "launch" case, but we could definitely come up with something better for the "attach" case. I think we can leave the attach code as-is for now, as the symmetry with the launch code is nice, but this will need to be revisited if the test turns out to be flaky.

This revision was automatically updated to reflect the committed changes.

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.