Fix #5279 Leaking java debug process #5281

tom-shan · 2019-05-28T11:02:26Z

When start debugging using vscode java-debug extension,
the debug process will remain in OS process list and
never be killed by theia process if the page is reloaded.

When terminate plugin server, kill all child processes
of plugin-host process.

Signed-off-by: tom-shan [email protected]

tom-shan · 2019-05-28T11:26:43Z

fix #5279

tom-shan · 2019-06-01T01:20:03Z

@benoitf @evidolob @akosyakov Would you mind reviewing this PR?

akosyakov · 2019-06-01T03:13:13Z

packages/plugin-ext/src/hosted/node/hosted-plugin-process.ts

@@ -120,13 +121,22 @@ export class HostedPluginProcess implements ServerPluginRunner {
            }
        });
        const hostedPluginManager = rpc.getProxy(MAIN_RPC_CONTEXT.HOSTED_PLUGIN_MANAGER_EXT);
+        this.killChildProcesses(cp.pid);


it should go in then case after after plugin got unloaded

akosyakov · 2019-06-03T06:12:48Z

I don't think it is really resolves the issue. Usually child process should observe parent process and if it exist then exit as well. Killing child processes from a parent process does not guarantee that it will happens always, e.g. if a parent process crashes it won't have a chance of doing it. I'm pretty sure that vscode java debug extension already has this logic, otherwise it won't work in VS Code as well. It bothers me why debug process exits only when a main server process dies. It seems that propagation of parent process id is done bogusly.

tom-shan · 2019-06-03T07:42:31Z

I have tested these two extensions in vscode(1.34.0) and got the similar results.
If I kill the extension host process in the background, then the java process will be killed later. But the java debug process will remain in the background even when I have exited the vscode.
If I click Start Debug, the debug console will show the same error.

akosyakov · 2019-06-03T07:52:42Z

@tom-shan you are right, just got the same

packages/plugin-ext/src/hosted/node/hosted-plugin-process.ts

akosyakov · 2019-06-03T10:00:24Z

It probably solves #4590
but we have to revert #5257 (@amiramw sorry, but it caused issue which were discussed on the PR)
and look into proper solution where plugins are unloaded from fronted and initializes on reconnection again (i am going to look into it later this week)

amiramw · 2019-06-03T12:08:37Z

Will this problem happen also when two clients connect together? Like two different browsers.
Is this scenario expected to work in general with the plugins mechanism?

akosyakov · 2019-06-04T05:04:36Z

Will this problem happen also when two clients connect together? Like two different browsers.
Is this scenario expected to work in general with the plugins mechanism?

I don't think it is related to the plugin system. When 2 users connect to the same backend they cannot expect to run anything on the same ports. Regarding debugging there is an issue to make debug session shared by default. That each client for the same workspace see all running debug sessions.

tom-shan · 2019-06-23T15:41:11Z

Hi @akosyakov, it seems like no other questions, could you help merge this PR?

kittaakos · 2019-07-25T14:25:26Z

packages/plugin-ext/src/hosted/node/hosted-plugin-process.ts

+        parentProcess.disconnect();
+        const parentPid = parentProcess.pid;
+        if (isWindows) {
+            cp.spawn('taskkill', ['/F', '/T', '/PID', parentPid.toString()]);


I had a very similar issue in one of my downstream projects. A few remarks on the PR:

Instead of doing taskkill on Windows and kill on *nix, just use tree-kill:

require('tree-kill')(yourPID);

You have to get the process tree on Windows as well. Not just in the else branch.

interesting that VS Code uses taskkill for it: https://github.com/microsoft/vscode/blob/a848f18df4efa9eb947bd01cbebcede418e66148/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L269-L275

ah, i see tree-kill does it https://github.com/pkrumins/node-tree-kill/blob/master/index.js#L20

Thanks for your recommendation, I will try to use this new npm package.

@tom-shan, please ping if you are done, I'll try to find time for the verification. Thanks!

akosyakov · 2019-07-26T07:01:33Z

@tom-shan sorry, we still had not time to test this PR on different platforms

@kittaakos could we rebase, test it as it is and merge? or you will prefer to use tree-kill

Could you verify on windows please?

In order to test one can install nodejs vscode extensions (https://marketplace.visualstudio.com/items?itemName=ms-vscode.node-debug + https://marketplace.visualstudio.com/items?itemName=ms-vscode.node-debug2) instead of native (@theia/debug-nodejs) and then ~~kill the backend~~ close the window during debugging and verify that processes are gone. You will need to wait a bit because of #5257.

tom-shan · 2019-07-26T15:51:08Z

@kittaakos @akosyakov Please have a review.

kittaakos · 2019-07-29T09:08:39Z

Please have a review.

Sorry for the confusion, @tom-shan. I think what you did for the non-Windows version was the right way: you have to kill the children of the process you want to terminate, then the process. Something like this:

private killChildProcesses(parentPid: number): void {
    psTree(parentPid, (_, children) {
        for (const { PID } of children) {
            kill(PID);
        }
        kill(parentPid);
    });
}

tom-shan · 2019-07-29T12:16:40Z

But according to https://github.com/pkrumins/node-tree-kill/blob/master/index.js#L47-L56, it will kill child processes first.
BTW, https://www.npmjs.com/package/terminate may be a better choice?

kittaakos · 2019-07-29T12:22:10Z

But according to https://github.com/pkrumins/node-tree-kill/blob/master/index.js#L47-L56, it will kill child processes first.

The logic you have referenced does not run for Windows: https://github.com/pkrumins/node-tree-kill/blob/3b5b8feeb3175a3e16ea7e0e09fdf5b8d2b87b08/index.js#L20-L21

tom-shan · 2019-07-29T13:08:13Z

Got it. So we can use 'terminate' instead, because it has the right killing order and you can refer to https://github.com/dwyl/terminate/blob/master/handlePsTreeCallback.js#L17

kittaakos · 2019-07-29T13:16:14Z

we can use 'terminate'

I have not used it yet, but if you want to use terminate, add it, I will verify.

tom-shan · 2019-07-29T15:49:14Z

I'm done.

kittaakos · 2019-07-31T06:14:36Z

I am verifying this on Windows.

akosyakov · 2019-07-31T07:10:54Z

please don't forget to check licenses of new dependencies: https://github.com/theia-ide/theia/wiki/Registering-CQs#wip---new-ecd-theia-intellectual-property-clearance-approach-experimental

cc @marcdumais-work

kittaakos · 2019-07-31T07:57:17Z

I tried to verify it on Windows in the electron example app. It did not work. I did the followings:

Removed @theia/java and @theia/java-debug from both the browser and electron package.json files.
Rebuilt the extensions without Theia Java.
Downloaded the two Java VS Code extensions.

root INFO unzipping the plugin ProxyPluginDeployerEntry {
  deployer:
   PluginVsCodeFileHandler {
     unpackedFolder: 'C:\\Users\\KITTAA~1\\AppData\\Local\\Temp\\vscode-unpacked' },
  delegate:
   PluginDeployerEntryImpl {
     originId: 'local-dir:../../plugins',
     pluginId: 'redhat.java-0.47.0.vsix',
     map: Map {},
     changes: [],
     acceptedTypes: [],
     currentPath:
      'C:\\Users\\kittaakos\\dev\\theia\\plugins\\redhat.java-0.47.0.vsix',
     initPath:
      'C:\\Users\\kittaakos\\dev\\theia\\plugins\\redhat.java-0.47.0.vsix',
     resolved: true,
     resolvedByName: 'LocalDirectoryPluginDeployerResolver' },
  deployerName: 'PluginVsCodeFileHandler' }
root INFO unzipping the plugin ProxyPluginDeployerEntry {
  deployer:
   PluginVsCodeFileHandler {
     unpackedFolder: 'C:\\Users\\KITTAA~1\\AppData\\Local\\Temp\\vscode-unpacked' },
  delegate:
   PluginDeployerEntryImpl {
     originId: 'local-dir:../../plugins',
     pluginId: 'vscjava.vscode-java-debug-0.20.0.vsix',
     map: Map {},
     changes: [],
     acceptedTypes: [],
     currentPath:
      'C:\\Users\\kittaakos\\dev\\theia\\plugins\\vscjava.vscode-java-debug-0.20.0.vsix',
     initPath:
      'C:\\Users\\kittaakos\\dev\\theia\\plugins\\vscjava.vscode-java-debug-0.20.0.vsix',
     resolved: true,
     resolvedByName: 'LocalDirectoryPluginDeployerResolver' },
  deployerName: 'PluginVsCodeFileHandler' }
root INFO PluginTheiaDirectoryHandler: accepting plugin with path C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\r
edhat.java-0.47.0.vsix
root INFO Resolved "redhat.java-0.47.0.vsix" to a VS Code extension "[email protected]" with engines: { vscode: '^1.36.0' }
root INFO PluginTheiaDirectoryHandler: accepting plugin with path C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\v
scjava.vscode-java-debug-0.20.0.vsix
root INFO Resolved "vscjava.vscode-java-debug-0.20.0.vsix" to a VS Code extension "[email protected]" with engine
s: { vscode: '^1.32.0' }
root INFO Deploying backend plugin "[email protected]" from "C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\redhat.java-
0.47.0.vsix\extension\dist\extension"
root INFO Deploying backend plugin "[email protected]" from "C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked
\vscjava.vscode-java-debug-0.20.0.vsix\extension\dist\extension"

Cloned https://github.com/spring-projects/spring-petclinic and opened it as my workspace.

root INFO [nsfw-watcher: 2572] Started watching: c:\Users\kittaakos\dev\spring-petclinic\.vscode\launch.json
root INFO [nsfw-watcher: 2572] Started watching: c:\Users\kittaakos\dev\spring-petclinic\.vscode\settings.json
root INFO Started watching the git repository: file:///c%3A/Users/kittaakos/dev/spring-petclinic

Waited for the LS to start and build.

root INFO [hosted-plugin: 3152] PLUGIN_HOST(3152): PluginManagerExtImpl/init()
PLUGIN_HOST(3152): initializing([email protected] with C:\Users\kittaakos\dev\theia\packages\plugin-ext-vscode\lib\node/plugin
-vscode-init.js)
PLUGIN_HOST(3152): initializing([email protected] with C:\Users\kittaakos\dev\theia\packages\plugin-ext-vscode\li
b\node/plugin-vscode-init.js)
Debugger contribution has been registered: java
root INFO [hosted-plugin: 3152] PLUGIN_HOST(3152): PluginManagerExtImpl/loadPlugin(C:\Users\KITTAA~1\AppData\Local\Temp\
vscode-unpacked\redhat.java-0.47.0.vsix\extension\dist\extension)
root INFO [hosted-plugin: 3152] { message:
   'Starting Java server with: C:\\PROGRA~1\\Java\\JDK18~1.0_1\\bin\\java -Declipse.application=org.eclipse.jdt.ls.core.
id1 -Dosgi.bundles.defaultStartLevel=4 -Declipse.product=org.eclipse.jdt.ls.core.product -Dfile.encoding=UTF-8 -DwatchPa
rentProcess=false -noverify -Xmx1G -XX:+UseG1GC -XX:+UseStringDeduplication -jar C:\\Users\\KITTAA~1\\AppData\\Local\\Te
mp\\vscode-unpacked\\redhat.java-0.47.0.vsix\\extension\\server\\plugins\\org.eclipse.equinox.launcher_1.5.500.v20190715
-1310.jar -configuration C:\\Users\\KITTAA~1\\AppData\\Local\\Temp\\vscode-unpacked\\redhat.java-0.47.0.vsix\\extension\
\server\\config_win -data C:\\Users\\kittaakos\\AppData\\Roaming\\.theia\\workspace-storage\\b57d6bdc8a5ce6f798acacd19bf
4aa59\\redhat.java\\jdt_ws',
  level: 'info',
  timestamp: '2019-07-31 09:39:52.756' }

Started the Debug (Launch)-PetClinicApplication<spring-petclinic> launch configuiration.
Had an error:

root INFO [hosted-plugin: 3152] PLUGIN_HOST(3152): PluginManagerExtImpl/loadPlugin(C:\Users\KITTAA~1\AppData\Local\Temp\
vscode-unpacked\vscjava.vscode-java-debug-0.20.0.vsix\extension\dist\extension)
root INFO [hosted-plugin: 3152] Debug configuration provider has been registered: java
root INFO [hosted-plugin: 3152] Cannot update debug settings. { Error: No delegateCommandHandler for vscode.java.updateD
ebugSettings
    at C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\redhat.java-0.47.0.vsix\extension\dist\extension.js:1:13690
    at C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\redhat.java-0.47.0.vsix\extension\dist\extension.js:1:13984
    at Immediate.k.size.setImmediate (C:\Users\KITTAA~1\AppData\Local\Temp\vscode-unpacked\redhat.java-0.47.0.vsix\exten
sion\dist\extension.js:1:14345)
    at runCallback (timers.js:696:18)
    at tryOnImmediate (timers.js:667:5)
    at processImmediate (timers.js:649:5) code: -32601, data: undefined }
root ERROR Error starting the debug session Error: It is not possible to provide debug adapter executable.
    at DebugExtImpl.<anonymous> (C:\Users\kittaakos\dev\theia\packages\plugin-ext\lib\plugin\node\debug\debug.js:501:39)
    at step (C:\Users\kittaakos\dev\theia\packages\plugin-ext\lib\plugin\node\debug\debug.js:32:23)
    at Object.next (C:\Users\kittaakos\dev\theia\packages\plugin-ext\lib\plugin\node\debug\debug.js:13:53)
    at fulfilled (C:\Users\kittaakos\dev\theia\packages\plugin-ext\lib\plugin\node\debug\debug.js:4:58)
    at process._tickCallback (internal/process/next_tick.js:68:7)

akosyakov · 2019-07-31T08:55:18Z

@kittaakos Please try with node.js vscode extensions, they are in the better state.

In order to test one can install nodejs vscode extensions (https://marketplace.visualstudio.com/items?itemName=ms-vscode.node-debug + https://marketplace.visualstudio.com/items?itemName=ms-vscode.node-debug2) instead of native (@theia/debug-nodejs) and then kill the backend close the window during debugging and verify that processes are gone. You will need to wait a bit because of #5257.

marcdumais-work · 2019-07-31T10:46:19Z

please don't forget to check licenses of new dependencies: https://github.com/theia-ide/theia/wiki/Registering-CQs#wip---new-ecd-theia-intellectual-property-clearance-approach-experimental

cc @marcdumais-work

+1

terminate is licensed under GPL v2.0 and so is not appropriate for an Eclipse Foundation project such as Theia.

tom-shan · 2019-07-31T12:50:35Z

OK, I will refactor this PR without terminate.

tom-shan · 2019-07-31T13:40:44Z

@kittaakos Please verify it again.

packages/plugin-ext/package.json

kittaakos · 2019-07-31T14:04:25Z

Please try with node.js vscode extensions

@akosyakov, how can I make sure it is a fix for the leaking Java process?

When start debugging using vscode java-debug extension, the debug process will remain in OS process list and never be killed by theia process if the page is reloaded. When terminate plugin server, kill all child processes of plugin-host process. Signed-off-by: tom-shan <[email protected]>

tom-shan · 2019-07-31T15:00:16Z

I tried to verify it on Windows in the electron example app. It did not work. I did the followings:

I have tested with two vscode extensions: java 0.44.0 and java-debug 0.15.0 in Ubuntu and have the
same error of root ERROR Error starting the debug session Error: It is not possible to provide debug adapter executable.

kittaakos · 2019-07-31T15:19:37Z

have the
same error of

Thanks for the note, @tom-shan.

Locally, I have rebased your code from the HEAD of Theia's master. Maybe, it was fixed.
If no, I will check with the exact same version you provided in the original issue description. Very solid steps, by the way 👍🎖

tom-shan · 2019-07-31T15:36:53Z

If still have the same error, you can try with another repo which is much simpler and the debug session is started successfully.

tom-shan · 2019-08-01T14:11:16Z

@kittaakos After the extra process of cleaning some data(~/.theia and /tmp/vscode-unpacked), the error of root ERROR Error starting the debug session Error: It is not possible to provide debug adapter executable is gone and the debug session can be started successfully in my vm.

kittaakos

I have verified it on Windows in electron. It worked. I started several Java debug processes, and when I refreshed the browser window, all Java processes were terminated. 👍 Thank you for your help on this, @tom-shan!

kittaakos · 2019-08-02T07:01:20Z

Are we good license-wise for this PR, @marcdumais-work? Thanks!

marcdumais-work · 2019-08-02T11:18:13Z

Are we good license-wise for this PR, @marcdumais-work? Thanks!

LGTM - the latest version of this PR adds no new dependency (ps-tree was already pulled from @theia/plugin-dev.

kittaakos · 2019-08-02T11:19:01Z

LGTM

Thank you for your help!

tom-shan requested review from benoitf and evidolob as code owners May 28, 2019 11:02

tom-shan changed the title ~~fix leaking java debug process~~ Fix #5279 Leaking java debug process May 28, 2019

tom-shan force-pushed the debug-session branch 3 times, most recently from cdadffd to 534fb55 Compare May 31, 2019 10:57

akosyakov reviewed Jun 1, 2019

View reviewed changes

tom-shan mentioned this pull request Jun 1, 2019

[vscode]Leaking java debug process #5279

Closed

tom-shan force-pushed the debug-session branch from 534fb55 to f00607c Compare June 2, 2019 15:04

akosyakov reviewed Jun 3, 2019

View reviewed changes

packages/plugin-ext/src/hosted/node/hosted-plugin-process.ts Outdated Show resolved Hide resolved

tom-shan force-pushed the debug-session branch from f00607c to d569b88 Compare June 5, 2019 12:25

tom-shan force-pushed the debug-session branch from d569b88 to 5ecbe58 Compare June 23, 2019 15:30

kittaakos reviewed Jul 25, 2019

View reviewed changes

akosyakov added debug issues that related to debug functionality vscode issues related to VSCode compatibility labels Jul 26, 2019

tom-shan force-pushed the debug-session branch from 5ecbe58 to d48b272 Compare July 26, 2019 15:48

tom-shan force-pushed the debug-session branch 2 times, most recently from 89ca646 to 66e5127 Compare July 29, 2019 15:46

tom-shan force-pushed the debug-session branch from 66e5127 to 8484930 Compare July 31, 2019 13:30

kittaakos reviewed Jul 31, 2019

View reviewed changes

packages/plugin-ext/package.json Show resolved Hide resolved

tom-shan force-pushed the debug-session branch from 8484930 to 14c580a Compare July 31, 2019 14:33

kittaakos self-requested a review August 2, 2019 06:56

kittaakos approved these changes Aug 2, 2019

View reviewed changes

kittaakos mentioned this pull request Aug 2, 2019

[debug][java][windows][vs-code] Cannot start debug spring-petclinic #5840

Closed

kittaakos merged commit 49f80ff into eclipse-theia:master Aug 2, 2019

Fix #5279 Leaking java debug process #5281

Fix #5279 Leaking java debug process #5281

Conversation

tom-shan commented May 28, 2019

tom-shan commented May 28, 2019

tom-shan commented Jun 1, 2019

akosyakov Jun 1, 2019

Choose a reason for hiding this comment

tom-shan Jun 2, 2019

Choose a reason for hiding this comment

akosyakov commented Jun 3, 2019 • edited Loading

tom-shan commented Jun 3, 2019

akosyakov commented Jun 3, 2019

akosyakov commented Jun 3, 2019 • edited Loading

amiramw commented Jun 3, 2019

akosyakov commented Jun 4, 2019

tom-shan commented Jun 23, 2019

kittaakos Jul 25, 2019

Choose a reason for hiding this comment

akosyakov Jul 26, 2019

Choose a reason for hiding this comment

akosyakov Jul 26, 2019

Choose a reason for hiding this comment

tom-shan Jul 26, 2019

Choose a reason for hiding this comment

kittaakos Jul 26, 2019

Choose a reason for hiding this comment

akosyakov commented Jul 26, 2019 • edited Loading

tom-shan commented Jul 26, 2019

kittaakos commented Jul 29, 2019

tom-shan commented Jul 29, 2019

kittaakos commented Jul 29, 2019

tom-shan commented Jul 29, 2019

kittaakos commented Jul 29, 2019

tom-shan commented Jul 29, 2019

kittaakos commented Jul 31, 2019

akosyakov commented Jul 31, 2019

kittaakos commented Jul 31, 2019

akosyakov commented Jul 31, 2019

marcdumais-work commented Jul 31, 2019

tom-shan commented Jul 31, 2019

tom-shan commented Jul 31, 2019

kittaakos commented Jul 31, 2019

tom-shan commented Jul 31, 2019

kittaakos commented Jul 31, 2019

tom-shan commented Jul 31, 2019 • edited Loading

tom-shan commented Aug 1, 2019

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos commented Aug 2, 2019

marcdumais-work commented Aug 2, 2019

kittaakos commented Aug 2, 2019

akosyakov commented Jun 3, 2019 •

edited

Loading

akosyakov commented Jun 3, 2019 •

edited

Loading

akosyakov commented Jul 26, 2019 •

edited

Loading

tom-shan commented Jul 31, 2019 •

edited

Loading