Skip to content
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.

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

[DAP] Only kill positive pids on disconnection #55209

Open
elliette opened this issue Mar 15, 2024 · 1 comment
Open

[DAP] Only kill positive pids on disconnection #55209

elliette opened this issue Mar 15, 2024 · 1 comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@elliette
Copy link
Contributor

On disconnection, DAP kills all pids:

void terminatePids(ProcessSignal signal) {
// TODO(dantup): In Dart-Code DAP, we first try again with sigint and wait
// for a few seconds before sending sigkill.
for (var pid in pidsToTerminate) {
Process.killPid(pid, signal);
}
}

However, this set of pids contains the VM pid, which defaults to-1 if it was not set:

pid = json['pid'] ?? -1;

On Linux, killing -1 is a special case that means "kill all the process that you can." We had a case where this was killing a bunch of running processes on a users machine.

@elliette elliette added the dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation label Mar 15, 2024
@DanTup
Copy link
Collaborator

DanTup commented Mar 15, 2024

However, this set of pids contains the VM pid, which defaults to -1 if it was not set

I presume this is something mis-reporting it's PID, and not DAP recording a -1 somewhere that wasn't set? We should only record pids that are provided to DAP (such as in JSON requests from other tools, or from the a response from the VM Service).

It's not clear which of these payloads contains the offending -1 though (if it's somewhere where the field is optional, I wonder if it could be omitted instead of set to -1?)

copybara-service bot pushed a commit that referenced this issue Mar 16, 2024
This was causing a bug in Cider where if the VM didn't have a pid set, it defaulted to -1, and killing -1 causes Linux to kill everything that it can.

Bug: #55209
Change-Id: I2dae8cf8ecf718a209df9db33f81cecf6ee48d39
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357501
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Elliott Brooks <elliottbrooks@google.com>
@mit-mit mit-mit added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Mar 18, 2024
@pq pq added the P2 A bug or feature request we're likely to work on label Mar 18, 2024
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants