POSIX Build Stopping
Reporter: RobertBColton | Status: closed | Last Modified: July 04, 2020, 12:15:12 pmwaitpid
does not have an easy version that accepts a timeout and I was afraid sleeping would be a busy wait. I tried it on Linux myself and it seemed to work without high CPU usage, so I presume my concerns were unfounded. Regardless I opted to make it 10,000 microsecond sleep, or 10 milliseconds, or a hundreth of a second which is the same as the Win32 version I already merged.i like it! its a great feature. there have been times where memory leaks having occurred, there would've been no way to escape/come out short of rebooting the system. this eliminates doing that.
If pid is negative, but not -1, sig shall be sent to all processes (excluding an unspecified set of system processes) whose process group ID is equal to the absolute value of pid, and for which the process has permission to send a signal.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html
I want to also mention that I feel we are doing the right thing because CodeBlocks, Visual Studio, etc do it this way.
https://github.com/svn2github/CodeBlocks/blob/3d14f1de1aa769351b08eb7793082727f5f3cba9/src/plugins/compilergcc/compilergcc.cpp#L2906
However, I think CodeBlocks goes overboard with the use of SIGTERM and SIGKILL.
https://github.com/svn2github/CodeBlocks/blob/3d14f1de1aa769351b08eb7793082727f5f3cba9/src/plugins/compilergcc/compilergcc.cpp#L2914
I like how Visual Studio does a SIGINT first, then a SIGKILL/SIGTERM on the second press of stop if the first doesn't work.
However if the debugger hits a breakpoint in the shutdown code or if the debuggee does not terminate properly by itself, then the debug session will not end. In this case, pressing Stop again will force terminate the debuggee and its child processes (SIGKILL).
https://code.visualstudio.com/docs/nodejs/nodejs-debugging#_stop-debugging
Yes and no. It does call make but make just invokes gradle here:
https://github.com/enigma-dev/enigma-dev/blob/master/ENIGMAsystem/SHELL/Makefile#L26
The shell should also call setpgid to put each of its child processes into the new process group. This is because there is a potential timing problem: each child process must be put in the process group before it begins executing a new program, and the shell depends on having all the child processes in the group before it continues executing. If both the child processes and the shell call setpgid, this ensures that the right things happen no matter which process gets to it first.
https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html
setpgid(pid, pid);
tcsetpgrp(STDIN_FILENO, pid);
https://stackoverflow.com/a/36486131
Although, I will say, setsid
sounds a whole lot like DETACHED_PROCESS
because neither are immediately associated with a controlling terminal.
Merging #2070 into master will not change coverage.
The diff coverage isn/a
.
@@ Coverage Diff @@
## master #2070 +/- ##
=======================================
Coverage 30.93% 30.93%
=======================================
Files 197 197
Lines 19102 19102
=======================================
Hits 5910 5910
Misses 13192 13192
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 6fffee8...08e2ce1. Read the comment docs.
setsid
completely worked, it goes overboard with creating an entire session.https://cs.nyu.edu/courses/fall13/CSCI-UA.0201-003/lecture24.pdf
We could just detach the process, which the real equivalent of is double forking. Double forking is usually how daemons are created and it orphans the child process group from the original parent (ENIGMA). In essence, it detaches the new process.
In fact, QProcess::startDetached
uses double forking in the Qt Framework, which RGM already uses to launch the game.
https://sources.debian.org/src/qt4-x11/4:4.8.6+git64-g5dc8b2b+dfsg-3+deb8u1/src/corelib/io/qprocess_unix.cpp/#L1405
However, the funny thing is wxWidgets has wxEXEC_MAKE_GROUP_LEADER
for wxProcesses.
https://docs.wxwidgets.org/3.0/group__group__funcmacro__procctrl.html#gga1be3860693af99a6c1da72580097294ca433c517e46d1dd61a5dda1ffeb750552
They map it to setsid
on Unix.
https://github.com/wxWidgets/wxWidgets/blob/04f4ac32b149e89c9851b0a5ba1ad75792056993/src/unix/utilsunx.cpp#L659
wxWidgets maps it to CREATE_NEW_PROCESS_GROUP
on Windows.
https://github.com/wxWidgets/wxWidgets/blob/0ca45d1a59a51e6798b99eaa0e7fdbbcc33c3992/src/msw/utilsexc.cpp#L762
So it really looks like we can do either or, a double fork or put the group in a session. I still don't know which is better.
Regardless, because the process group I needed to effectively SIGINT interrupt becomes a background process group, any reading it does of STDIN generates SIGTTIN suspending the process. Then ENIGMA sets there waiting on it forever.
A process cannot read from the user’s terminal while it is running as a background job. When any process in a background job tries to read from the terminal, all of the processes in the job are sent a SIGTTIN signal. The default action for this signal is to stop the process.
https://www.gnu.org/software/libc/manual/html_node/Job-Control-Signals.html
It started on Android now and Ubuntu is ok to just close STDIN.
However, I didn't like that, because before this pull request make & the game both had access to LGM/ENIGMA's STDIN without being a process group. So, I decided to do what Stallman does and duplicate STDIN, then close the original STDIN to avoid the SIGTTIN signal.
https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html
It did not work when I tried to remove FD_CLOEXEC
from STDIN like Josh's STDOUT code was already doing.
That should mean that make & the game can read the LGM/ENIGMA STDIN through redirection. It's a simple solution, Android is building, it works in Ubuntu VM, so I am going with it.
/dev/null
as STDIN so that any attempts by Gradle to read it would give it the EOF/EOT and let it shutdown. This is pretty standard, and I didn't feel like doing all the busy work to redirect the current STDIN when we'll probably never need it.
I made this little program to demonstrate the problem I was having.
int infd = open("/dev/null", O_RDONLY);
dup2(infd, STDIN_FILENO);
//close(STDIN_FILENO); // <- read returns -1
char buf[1];
std::cout << "Done " << read(0, buf, 0) << std::endl;
std::cout << "buf " << buf[0] << std::endl;
return 0;
With that, the Travis Android job has passed and it's working great in my Ubuntu VM.
https://travis-ci.org/github/enigma-dev/enigma-dev/jobs/704857696#L4819
I want to point out that this is true about the Gradle source.
https://github.com/gradle/gradle/blob/838131e44c0271e9bc00995aaae33411429f31f4/subprojects/core/src/main/java/org/gradle/api/internal/tasks/userinput/DefaultUserInputReader.java#L39
If we had used the Daemon, that too would have worked, because it just makes a fake input stream that returns the same thing /dev/null
would have.
https://github.com/gradle/gradle/blob/162dfacbb728bc3c1b6da1b082bb2e9af6b169fd/subprojects/launcher/src/main/java/org/gradle/launcher/daemon/bootstrap/DaemonMain.java#L202
Anyway, I have to wait now on the MSYS2 people to fix the packages for the umpteenth time, so I'll double check this tomorrow.