Given the MR was merged can we close this @rgoldman ?
Yup, sorry about that and thank you for handling it.
I might have missed some detail in reviewing but I also think it looks good.
Two things:
"Anything PID related should also be fixed when 1.8.1 is released."
I think ABCL is working properly on all versions with the current implementation we have here. Don't ask me why, but what solved it was @rgoldman's declaim ftype for terminate-process.
Yup if one of you does a commit the CI passes.
What about squashing the whole MR? We don't want all these commits on master right?
Alberto Ferreira (bfa5f6d0) at 11 Nov 19:02
Refactor: Replace ecase with check-type.
Ping @rgoldman
Yes, it still works :)
Alberto Ferreira (4f4dcae8) at 05 Nov 19:00
Revert plan.lisp
It was making the tests fail, in some implementations it complained those were not defined, namely +good-bits+.
We can try reverting and see if there are issues again.
@rgoldman I think it's ready, will you test again on MacOS or now it's just merging?
(the only errors are the pathnames on ABCL because it ran on my fork)
ABCL works without target :process-group on all versions ;)
No worries :)
After your (declaim ftype) for terminate-process the ABCL bugs stopped I believe. I pushed a new commit without the expected failures (that were disabled already). Let's see if CI passes with :Target :pid for ABCL.
P.S:: Only your commits pass the pipeline, as mine still have those pathname issues on ABCL.
Alberto Ferreira (9144e823) at 27 Oct 19:10
Remove ABCL expected failures from termination tests
Alberto Ferreira (546cc31e) at 27 Oct 19:05
Remove ABCL expected failures from termination tests
Alberto Ferreira (c7ea34a2) at 26 Oct 18:41
Use format ~D instead of write-to-string
Alberto Ferreira (4b0e6985) at 26 Oct 18:37
Call kill instead of /bin/kill, respecting PATH
... and 1 more commit
@rgoldman can we close this one? what are your thoughts on this?
P.S @rgoldman : There's a possible race condition due to recycling PID's on the OS.
Imagine the process has died, and the OS is using many processes. After reaching the PID limit (32768 in some OS's), pid's will start being recycled. Meaning, the next launched process will spawn with the first free PID slot counting from 0. If it happens that it matches our already-dead process PID and we call uiop:terminate-process
, we'll send a signal to kill the new process when we thought we were killing our (already-dead) process.
Now I understand your question of the race condition. That part in the docstring is there before my changes. I don't know what they are referring to, but if I had to guess it comes from the actual Lisp implementation signalling methods.
We can exclude race conditions at the OS level, except for badly written programs with non-reentrant signal handlers, and there are never issues when a signal is sent once to terminate the process.
However, the launch-program/terminate-process layer in some implementations like Allegro for instance, uses some pipes being closed when the terminate-process is called and some signalling to the underlying process. By exclusion of parts, if there are any race conditions I can only assume they happen in the Lisp layer.
Do you confirm that I should remove the note from the dostring?
A signal-error is only called if the non-blocking signalling call itself raises an error or returns an error. Apart from that we're not raising errors.
For instance, some Lisp implementations' signalling calls do not raise signals, nor return errors, so for those, we can't detect that anything went wrong.
In our implementation, as it is non-blocking (we could add a :wait parameter and rethink things) even if terminate-process
says it succeeded, there's no guarantee that the signal was processed and the program quit. We are only saying that the signal was delivered. If urgent was used however, a SIGKILL is sent, meaning the program cannot handle it, and is forced to close.
Please tell me if there are still doubts