cderv commented 4 years ago

@yihui Following https://github.com/rstudio/blogdown/commit/64b41a81200c1807bdc6692f8feb8c3aa7bf614e, I think this need to be change also

https://github.com/rstudio/blogdown/blob/bf1f20081e5088046f4690992aca98ea2ee8ecc3/R/serve.R#L181-L187

Currently once hugo server is running, you can't stop it as announced with stop_server

This needs to be changed

cderv commented 4 years ago

And that is why I think I can't restard correctly because this function is called on the reg.finalizer and won't kill the processes.

cderv commented 4 years ago

I am noting that here as it is relevant to killing hugo process, and maybe a windows specificity:

On windows, I got two hugo process opened and one if the children of the other. It seems only one is returned in opts$get("pids"). One this one is killed the other is still alive. Killing the other opened pid though kill both.

Anyway, if both are not killed than RStudio won't be able to restart.

cderv commented 4 years ago

It seems only one is returned in opts$get("pids"). One this one is killed the other is still alive. Killing the other opened pid though kill both.

This is here: https://github.com/rstudio/blogdown/blob/bf1f20081e5088046f4690992aca98ea2ee8ecc3/R/serve.R#L211

Only the second is returned. That is not enough to kill all of them it seems. or at list the wrong one is returned because killig the pid shown in the message in the console don't kill both.

This would need to be changed to work ok on windows.

yihui commented 4 years ago

That's excellent testing! I have been puzzled for a long time this morning why the processes couldn't be killed and it often crashed my RStudio. With your discovery and fix, the problems were gone. I'll try to reproduce the problem of two hugo processes and fix it soon. Thanks a lot!

cderv commented 4 years ago

I'll keep testing further tomorrow with the fix re. killing process. It was painful to test further with that in place.

Re. the two process, why do you return only the second value ? Why not returning all pids, to all kill afterwards ?

Browse[3]> regmatches(pid2, m)
[[1]]
[1] "                     22100 " "22100"                      

[[2]]
[1] "                      1680 " "1680"

So instead of

m = regexec('\\s+([0-9]+)\\s+', pid2)
for (v in regmatches(pid2, m)) if (length(v) >= 2) return(v[2])

why not

sapply(regmatches(pid2, m), `[[`, 2)

to get all pids ?

Also, help page ?serve_site must be updated

yihui commented 4 years ago

Because in theory, there should only be a single hugo process and I don't want to accidentally kill other hugo processes (which might be started by other blogdown sites on the same computer). I don't know why two processes were spawned. I'll keep investigating.

yihui commented 4 years ago

I just pushed the fix for stop_server(), and I wasn't able to reproduce the issue of two Hugo processes. It might be due to the problem of stop_server(). You can install the current dev version and retry. It should be able to kill the process when the R session is ended or restarted. I only see one process in my Windows Task Manager, and it's killed successfully when I close RStudio.

cderv commented 4 years ago

I change of windows 10 computer and I still get two processes open. Here is what I see when I open a new blogdown project and that the hugo server is started.

I used ps 📦 to look at this from R as it allows to see which are the parent process.

Launching the server via the command:
  hugo server --bind 127.0.0.1 -p 4321 --themesDir themes -t hugo-lithium -D -F --navigateToChanged
Serving the directory . at http://127.0.0.1:4321
Launched the hugo server in the background (process ID: 21588). To stop it, call blogdown::stop_server() or restart the R session.
> blogdown:::opts$get("pids")
[1] "21588"
> p <- ps::ps_handle(21588L)
> ps::ps_parent(p)
<ps::ps_handle> PID=14508, NAME=rsession.exe, AT=2020-10-06 20:12:37
> ps::ps_children(p)
[[1]]
<ps::ps_handle> PID=9356, NAME=hugo.exe, AT=2020-10-06 20:12:44

> proc <- ps::ps()
> proc[proc$name == "hugo.exe", ]
# A tibble: 2 x 11
    pid  ppid name     username              status    user system      rss      vms created             ps_handle 
  <int> <int> <chr>    <chr>                 <chr>    <dbl>  <dbl>    <dbl>    <dbl> <dttm>              <I<list>> 
1  9356 21588 hugo.exe "DESKTOP-TITO\\chris" running 0.109  0.312  40984576 32780288 2020-10-06 20:12:44 <ps_handl>
2 21588 14508 hugo.exe "DESKTOP-TITO\\chris" running 0.0469 0.0312 12378112 12460032 2020-10-06 20:12:44 <ps_handl>
>

As you see

One hugo.exe has the R session as parent
Other hugo.exe has the other hugo.exe as parent

If I stop the server, only the second is close

> blogdown::stop_server()
> proc <- ps::ps()
> proc[proc$name == "hugo.exe", ]
# A tibble: 1 x 11
    pid  ppid name     username              status   user system      rss      vms created             ps_handle 
  <int> <int> <chr>    <chr>                 <chr>   <dbl>  <dbl>    <dbl>    <dbl> <dttm>              <I<list>> 
1  9356 21588 hugo.exe "DESKTOP-TITO\\chris" running 0.219  0.312 38846464 30318592 2020-10-06 20:12:44 <ps_handl>

Using a tool like ps or processx to handle this could help us identify the related processes and close only these ones.

Also closing RStudio and R session won't close the hugo process as it becomes orphan because the one remaining is the one that had the first hugo.exe as parent.

I'll continue to look into it tomorrow to understand why to processes are launched.

yihui commented 4 years ago

I ran your code and still didn't see the second hugo process. You may use the command

wmic process where "Name='hugo.exe'" get processid, commandline

to check the commands that started the hugo processes.

Or set a breakpoint here: https://github.com/rstudio/blogdown/blob/0bd67aea6a5316199516f33f1e4068d6226eef6e/R/serve.R#L220 and see if this line of code was executed twice for some reason, or it was executed once but launched two hugo processes.

yihui commented 4 years ago

I just pushed another possible fix. My guess is that your computer is much faster than my virtual machine---so fast that it called serve_site() twice in a short time for some reason. Please check if this fixes the problem. Thanks!

yihui commented 4 years ago

I just thought more about it, and I don't think 8811973 will fix it. It shouldn't be possible for serve_site() to run in parallel. Let's debug in real time on your computer tomorrow.

cderv commented 4 years ago

I continue digging while debugging step by step and I now have found the culprit. It is because I have Hugo installed via Scoop package manager. This tool use shims to provide binary in the PATH, so when calling hugo it is calling the shim that will run a .bat file to run the real hugo.exe binary. This is why I get to process call hugo.exe and why you can't reproduce.

The issue is that killing the master process (the shim one) won't kill the child process. However, interrupting will.

Using processx, proc$interrupt() will close all opened processes (parent and childs). Let's note that proc$kill() won't, but proc$kill_tree() will close all also.
Using proc_kill (using SIGTERM on windows) on the retreived ID won't close both processes. But it will on the child process - but that is not the pid retreived by blogdown.

So at the end, I think we can say it is an unsual case but it shows that launching and terminating background (especially on windows) is not easy. There could be child processes won't do know about and closing properly by just parsing command line results is not the easiest.

Some improvment I see:

I suggest if we rely on processx if available, we store the proc to use proc$interrupt() or proc$kill_tree() in stop_server()
When using bg_process we could use wmic or powershell to try find all the process though request. Not the easiest but feasible (exemple: wmic process where "ProcessId=8324 OR ParentProcessId=8324" get processid, parentprocessid)

Would that type of issue happen on OSX or UNIX ? the shim stuff ? I did not try with chocolatey, but I will to see if this is only happening with Scoop.

cderv commented 4 years ago

I did not try with chocolatey, but I will to see if this is only happening with Scoop.

I have just tried and as I thought this is also an issue with hugo installed by chocolatey. As it is also using shims, this is not really a surprise.

cderv commented 4 years ago

And I confirm this is working correctly when hugo is installed from R install_hugo

yihui commented 4 years ago

taskkill /t should kill a process and its child processes on Windows. For *nix, kill -PGID does the same job. This should be fixed finally.

cderv commented 4 years ago

When using hugo install by Scoop, I confirm that blogdown::stop_server() will now close all processes using processx or not.

There is still an issue when restarting R: I guess that follows my comment on https://github.com/rstudio/blogdown/commit/64d5f46ad0e38a9b399661ca1bcd465763a450bf#commitcomment-43062896

I think processx is killing the main process without the child, and reg.finalizer in bookdown does nothing.

I'll check that and suggest a fix

cderv commented 4 years ago

have you tested on Rstudio server on linux ?

Does not seem to work well either (using the last version on Github). Either on a VM locally or in RStudio cloud. stop_server() is just hanging... 😞

cderv commented 4 years ago

For *nix, kill -PGID does the same job. This should be fixed finally.

Ok I believe this is because on linux, the PGID you get will by doing ps -o pgid= <pid> is the pid of the rsession. So when you call kill -- -<pgid> your are also killing the current R session.

So it is not currently the same as on Windows because you are not killing the parent of the hugo process which is also the rsession.

The new linux way introduced in https://github.com/rstudio/blogdown/commit/3194cd1a0a0d72e788d2748381178cc7b1c3a3ef is too much.

Example

Hugo server is run (PID 1849)

> blogdown:::preview_site(startup = TRUE)
Launching the server via the command:
  /home/cderv/bin/hugo server --bind 127.0.0.1 -p 4321 --themesDir themes -t hugo-lithium -D -F --navigateToChanged
Start building sites … 

                   | EN  
-------------------+-----
  Pages            | 20  
  Paginator pages  |  0  
  Non-page files   |  0  
  Static files     | 15  
  Processed images |  0  
  Aliases          |  0  
  Sitemaps         |  1  
  Cleaned          |  0  

Built in 7 ms
Watching for changes in /home/cderv/test-blogdown/{content,static,themes}
Watching for config changes in /home/cderv/test-blogdown/config.toml
Environment: "development"
Serving pages from memory
Running in Fast Render Mode. For full rebuilds on change: hugo server --disableFastRender
Web Server is available at //localhost:4321/ (bind address 127.0.0.1)
Press Ctrl+C to stop
Serving the directory . at http://127.0.0.1:4321
Launched the hugo server in the background (process ID: 1849). To stop it, call blogdown::stop_server() or restart the R session.

but in terminal

(base) cderv@DESKTOP-D24Q6VQ:~/test-blogdown$ ps xao pid,ppid,pgid,sid,comm
  PID  PPID  PGID   SID COMMAND
    1     0     0     0 init
    6     1     6     6 init
    7     6     6     6 init
    8     7     8     8 bash
  128     7   128   128 rserver
 1769   128  1769   128 rsession
 1796  1769  1796  1796 bash
 1849     7  1769   128 hugo
 1869  1796  1869  1796 ps
(base) cderv@DESKTOP-D24Q6VQ:~/test-blogdown$ ps -o pgid= 1849
 1769

You got a pgid of 1769, but as you can see it includes the R session. So killing the group will kill the R Session, and RStudio server does not like that obviously.

cderv commented 4 years ago

Just to precise the above is with bg_process only, not when using processx.

I believe this is because system2 is launching a background process as a child of the current R session. It appears processx is not.

this is when processx is activated

> ps xao pid,ppid,pgid,sid,comm
  PID  PPID  PGID   SID COMMAND
    1     0     0     0 init
    6     1     6     6 init
    7     6     6     6 init
    8     7     8     8 bash
  128     7   128   128 rserver
 2258   128  2258   128 rsession
 2329  2258  2329  2329 hugo
 2350  2258  2350  2350 bash
 2382  2350  2382  2350 ps

hugo as its own pgid in this case. It means restarting or calling stop_server will not crash the R session.

yihui commented 4 years ago

You are correct. I didn't find this problem because it was not reproducible on macOS. I just tested it on Ubuntu, and stop_server() did crash the R session. I'll look for a safer way to kill the process and its child processes. Thanks!

yihui commented 4 years ago

I can't say "fixed finally" this time 😂 Let me know if you still have problems...

cderv commented 4 years ago

I think this is good now. no issue today ! Well done!

rstudio / blogdown

Also change `stop_server` because hugo server is used now #480

Example