Use taskkill for win32

[renkun-ken]

What problem did you solve?

tree-kill still sometimes does not work in Windows. This PR follows the suggestion at #918 (comment) and use taskkill to kill spawned subprocesses.

[renkun-ken]

@Yunuuuu Would you like to try the latest build here? It seems sub-process created by cp.spawn could be correctly killed by taskkill called by cp.spawn in Windows. I tried in Windows and it works as expected.

[renkun-ken]

I test it on Windows and it works nicely.

[Yunuuuu]

Thanks you, @renkun-ken, I have built it and tested it following the contribution guideline of vscode-r. But maybe my operation is wrong, I'm not familiar with the building procedure.

My tests (maybe wrong, I'll test it again if the extension file release) suggested it remain here, but it indeed killed two child-process (R for Windows front-end):

the previous results:

I have searched taskkill help document, which shows examples for killing multiple PIDs:
taskkill /pid 1230 /pid 1241 /pid 1253. The code cp.spawn('taskkill', ['/pid', String(proc.pid), '/f', '/t']) seems only using one parameter /pid option for proc.pid (I don't know if the variable proc.pid has multiple values) variable. I'm poor in this language used in vscode extension, but something alike in R may looks like these (I have used these to kill the child-process successfully, hope these can be helpful):

            system2(
                "taskkill",
                # a element of this character vector correspond to a argument in `taskkill`
                # like: c("/pid 1230", "/pid 1241", "/pid 1253", "/t", "/f")
                args = c(paste("/pid", proc.pid), "/t", "/f"), 
                stdout = FALSE, stderr = FALSE,
                wait = FALSE
            )

I really appreciate you and your great help in maintaining such excellent extension.

[renkun-ken]

You might need to manually clean up the existing orphan processes, download the build, install it and restart vscode, and test it.

[renkun-ken]

As for the taskkill usage, the code here is for each spawned process, its tree will be killed by taskkill on dispose on windows. There's no need to write multiple pids there.

[Yunuuuu]

Thanks a lot @renkun-ken, I tested it again and I have re-boot my computer, vscode-r still only killed the R for Windows front-end
before quit:

after quit:

[Yunuuuu]

It seems this didn't kill the child-process as the R.term (R for Windows terminal front-end) should be the child of R.exe (R for Windows front-end). Strangely.

[renkun-ken]

It is quite strange that it works perfectly in my azure Windows environment but not in yours.

[Yunuuuu]

@renkun-ken, this can work!! But we should change the vscode-r setting and abandon R.exe: "r.rpath.windows": "the\\path\\of\\Rterm.exe". Thanks a lot!

[renkun-ken]

I test on an azure Windows environment with r.rpath.windows left blank (which is default) so it will fallback to the default behavior, i.e. find R from PATH environment and registry. And this works as expected.

[renkun-ken]

And in vscode-R, we use R from PATH, or bin\R.exe specified in registry key Computer\HKEY_LOCAL_MACHINE\SOFTWARE\R-core\R\InstallPath (e.g. C:\Program Files\R\R-4.1.2) in Windows.

Therefore, I'm actually testing with R.exe and it works as expected. Seems there's no need to explicitly use Rterm.exe in the settings.

[Yunuuuu]

Really appreciate you @renkun-ken ! I tested the latest vscode-r extension using tree-kill, and it could work well too. Maybe people in #918 should also change their r.rpath.windows setting.

[Yunuuuu]

I didn't register my R version in my registry key since I sometimes need different R version to reproduce my previous work.

[renkun-ken]

I tested the latest vscode-r extension using tree-kill, and it could work well too

I revert to using tree-kill in windows. Would you like to test the latest build here and see if it works?

I tested it and it works well too.

[renkun-ken]

@beansrowning Would you like to test the build from this PR by downloading the artifact at https://github.com/REditorSupport/vscode-R/pull/936/checks?

[Yunuuuu]

This can work for me

[renkun-ken]

Then I think we could merge this now.

[albertosantini]

Thanks for your efforts.
Eventually waiting for a new release. ;)

[leo-liar]

Thanks for your efforts. As I just tried the artifact from here and it still didn't work just for clarification:

Do I have to set r.rterm.windows and/or r.rpath.windows?

(My installation of R is also not registered within the Windows Registry but I manually added the executable's path to the %PATH% environment variable which for most intents and purposes just works.)

[renkun-ken]

@leo-liar In my testing, it looks like it might not work properly once the downloaded build is installed and even after a restart. You might need to restart vscode and try again?

Do I have to set r.rterm.windows and/or r.rpath.windows?

In my testing, I just use the default behavior: not setting r.rterm.* and r.rpath.*. In your case, your R should be properly found in the PATH.

[albertosantini]

Sigh. Tested the artifact and here it doesn't work. Shall I help you?

[leo-liar]

In my testing, I just use the default behavior: not setting r.rterm.* and r.rpath.*. In your case, your R should be properly found in the PATH.

@renkun-ken Thanks. Unfortunately it doesn't work, wether I configure the r.rterm.*/r.rpath.* or not. (And yes, I did restart VSCode after installing the build artifact.)

[Yunuuuu]

@leo-liar, Try to use Rterm.exe in your r.rpath.windows setting and don't use R.exe

[leo-liar]

Try to use Rterm.exe in your r.rpath.windows setting and don't use R.exe

This actually works!! (At least while using the artifact.)

Opening an .r file spawns a few Rterm.exe processes (though no more R.exe processes), closing said file terminates all but one, which is later killed when I close VSCode. If I exit VSCode all of them are killed immediately.

When I open an R terminal within VSCode this correctly spawns an R.exe process which is correctly killed after closing this terminal.

[renkun-ken]

From the documentation:

B.2 Invoking R under Windows
There are two ways to run R under Windows. Within a terminal window (e.g. cmd.exe or a more capable shell), the methods described in the previous section may be used, invoking by R.exe or more directly by Rterm.exe. For interactive use, there is a console-based GUI (Rgui.exe).

I don't see an explanation about the difference between R.exe and Rterm.exe. Looks like R.exe spawns Rterm.exe internally but in your case the Rterm.exe process is somehow detacted from the process tree of R.exe?

[renkun-ken]

Maybe we should default to using Rterm.exe to spawn R processes directly in Windows?

[Yunuuuu]

I don't know the reason, but we can only make it work when using Rterm.exe in r.rpath.windows setting, if we used R.exe the child R.term process created by R.exe won't be killed

[leo-liar]

Looks like R.exe spawns Rterm.exe internally but in your case the Rterm.exe process is somehow detacted from the process tree of R.exe?

Yes, this is exactly what I always observed. I just don't understand why the detaching happens. But setting Rterm.exe up via r.rpath.windows seems to be a viable solution.

[renkun-ken]

It is quite strange. In my case, I use straight R.exe and everything works well. Not sure where the inconsistency comes from. What version of R are you using?

[leo-liar]

I'm on 4.4.1 ("Kick Things") ...

[renkun-ken]

@Yunuuuu does the latest build here work for you if you don't use Rterm.exe? And what version of R are you using?

[Yunuuuu]

I tested again, it doesn't work. my R version is 4.1.1 (2021-08-10)

[albertosantini]

I confirm Rterm.exe in r.rpath.windows works:

• "r.rpath.windows": "c:\woot\opt\r\bin\x64\Rterm.exe"

My context:

• Windows 10 - Version 21H2
• VSCode 1.63.2
• R extension 2.3.4
• R 4.1.2
• languageserver 0.3.12
• Are you using multi-root workspace - No
• editing untitled files - No
• files outside your workspace root - No
• R_LANGSVR_POOL_SIZE=0 - in .Renviron
• "terminal.integrated.enablePersistentSessions": false - in VSCode settings
• "terminal.integrated.persistentSessionReviveProcess": "never" - in VSCode settings

[Yunuuuu]

I found some discussions (r-lib/callr#34 and r-lib/processx#96) which looks like similar to this

So, the problem is that callr starts R.exe on Windows, and this starts another R process, RTerm.exe I think.
processx/callr kills the R.exe process, but not the other RTerm.exe process, and probably because we are reading output from the Rterm.exe process (the file objects are passed to it), processx::run (and thus callr::r) keeps running.
The solution would be to start Rterm.exe directly, instead or R.exe, I think. That seems to work just fine, and needs to be fixed in callr.

[Yunuuuu]

when we use R.exe, vscode-r will only kill the R.exe and keep the child process Rterm.exe alive

[leo-liar]

I don't have any more R.exe processes if I set up Rterm.exe as described.

[renkun-ken]

I tried typing R.exe in powershell and 5 processes were created, cmd.exe .../bin/x64/R.exe, cmd.exe .../bin/x64/Rterm.exe, bin/R.exe, bin/x64/R.exe, bin/x64/Rterm.exe.

When I type taskkill /pid {pid} /t /f to kill the initial bin/R.exe process, all five processes are killed. The taskkill approach should work though.

[Yunuuuu]

Yes, I also tested taskkill can work to kill R.exe and their child process Rterm.exe by using taskkill directly in powershell or implementing it in R system2 command, but it couldn't work in vscode-r extension.

[albertosantini]

I am sorry, but after letting vscode running few minutes (until now since last try) I saw those processes "outside" vscode and when I close the ide, those processes are not closed. Really really weirdly.

[renkun-ken]

@Yunuuuu @albertosantini @leo-liar

I open a new PR #941 where taskkill command is used instead of tree-kill. I test it, and it always works without specifying Rterm.exe. Would you like to test it again and see if it behaves consistently across your environments?

[leo-liar]

I am sorry, but after letting vscode running few minutes (until now since last try) I saw those processes "outside" vscode and when I close the ide, those processes are not closed. Really really weirdly.

@albertosantini I let VSCode running for roughly 30mins now and I could not observe this behaviour.

---

I open a new PR #941 where taskkill command is used instead of tree-kill.

1. Will try as soon as the artefact is ready!
2. Worked at first try. (Rterm.exe still being configured in settings.)
3. Doesn't work anymore if I comment out the "r.rpath.windows": "Rterm.exe", line.

Another question:
Do you have a way to invoke Rterm directly within your extension? I mean as there are both r.rterm.* and r.rpath.* configuration options within VSCode and both executables reside within the same path (usually I think) and because correctly terminating all child processes works when Rterm.exe instead of R.exe is invoked, maybe trying always (or for first try) to internally call Rterm may make setting r.rpath.* directly to Rterm.exe superfluous?

From the documentation: At the Windows command-line, Rterm.exe is preferred to R.