Fix jekyll serve --detach with jekyll-sass-converter 3.x#9304
Fix jekyll serve --detach with jekyll-sass-converter 3.x#9304jekyllbot merged 2 commits intojekyll:masterfrom ntkme:daemon
jekyll serve --detach with jekyll-sass-converter 3.x#9304Conversation
at_exit hooks when --detach
at_exit hooks when --detachat_exit hooks when --detach
|
@ashmaroli This is a bug related to jekyll-sass-converter 3.x upgrade. Would you mind take a look? |
|
Hello @ntkme, |
Because this involves forking, we will have to write a test to actually launch a real Jekyll process, capture and parse the pid from stdout, and then check main pid has stopped with exit code 0 and new pid is running.
After forking, the old main process will exit normally as it has nothing more to do. The old version does not have any |
Yes @ntkme, that is the basic idea. Will you be able to translate that into code? If the current implementation (of the lib code) doesn't have a suitable interface for testing, you may extract the entire conditional branch into a new (preferably private) method for easier testing. |
at_exit hooks when --detachjekyll serve --detach with jekyll-sass-converter 3.x
|
@ashmaroli Test added. Also fixed another issue discovered in test when |
| # Detach the process from controlling terminal | ||
| $stdin.reopen("/dev/null", "r") | ||
| $stdout.reopen("/dev/null", "w") | ||
| $stderr.reopen("/dev/null", "w") |
There was a problem hiding this comment.
Is it possible to switch this out with the Open3 module?
There was a problem hiding this comment.
If we want to have a simpler or cleaner implementation we have to change the semantics of how this work.
Currently, --detach runs these steps:
- Build the site.
- Fork into child process and start server.
- Print the child process PID.
- Exit main process.
We have several options but they would change the semantics:
The simplest solution is to use Process.daemon. It is a single call that does fork and detach stdio then continue rest of the code in forked process. However:
- If we call
Process.daemonon main, the problem is that we cannot get and print PID in main process. A typical practice of using this method would be adding a--pid-fileoption to let forked process to write its PID into a file. - We can also call
Process.daemonin the forked process to use it for detach stdio, the problem is that it would fork again and change PID, which is why I had to manually do that by reopen stdio on/dev/null. For reference, here is CRuby's implementation inrb_daemonfor doing the same thing: https://github.com/ruby/ruby/blob/976cc3852b00560e48fe89b9c5ab77761e1d8f3b/process.c#L7111-L7115
Therefore, if we stick with fork based implementation, this PR is the best we can do unless we agree to change the semantics of how this option works today.
Another option would be replace the fork with a real ruby process that just runs webrick, with two different possible implementations:
- Serialize webrick options and start webrick with a ruby one-liner using
Process.spawn(oropen3). Problem is that we have some Ruby code that customize webrick behavior which makes serialization difficult. - Spawn a Jekyll process with
--skip-initial-buildwith the same ARGV that was originally used, so that it just start the server. Problem is that today we don't keep original ARGV after processing.
There was a problem hiding this comment.
A huge thank you for giving me a detailed, well-thought out reply, @ntkme 🙏 🙇
Since I don't intend to change much about this, especially since this is particularly unfamiliar territory for me, the current implementation here will do.
| pid = re.match(output)["pid"].to_i | ||
| assert pid > 1 | ||
|
|
||
| Process.kill 9, pid |
There was a problem hiding this comment.
This will raise if the process has already exited but that's fine since we'd likely want a test failure anyway.
|
@jekyllbot: merge +bug |
なつき: Fix `jekyll serve --detach` with jekyll-sass-converter 3.x (#9304) Merge pull request 9304
This is a 🐛 bug fix.
Summary
This PR fixes the issue that
Process.fork+Process.detachwould not detach correctly and may crash ruby when subprocess of sass-embedded (part of jekyll-sass-converter) is still running. The problem is that sass-embedded runs a child process and registers anat_exithook to stop the child process, and theat_exithook ran when main process exits but also get inherited by the forked process and would run again when forked process exits.This PR changes it so that any
at_exithook would not run when main process exits, but then would only run when the forked process exits.Context