Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Foreground Job Control Overhaul #661

Merged
merged 8 commits into from Dec 31, 2017

Conversation

@mmstick
Copy link
Collaborator

commented Dec 31, 2017

Job control in Ion was a complete mess, and it was a wonder that it even worked at one point to begin with! I've gone through and fixed a number of issues related to foreground job management.

  • Processes are no longer preemptively killed by Ion to emulate SIGPIPE support.
  • Processes within a pipeline are now assigned the same PGID.
  • A SIGPIPE handler is now registered so that processes exit when they receive a SIGPIPE.
  • Jobs should exit with the correct exit statuses in more situations.
  • External commands now use fork + exec, rather than std::process::Command.
  • Redox's foreground job management was also improved.
  • Some refactoring was performed, with more refactoring planned in the future.

There's likely some issues that remain to be addressed though. Needs more testing & bug reports. Background job management remains to be mostly broken for now, so that's an area to look into next.

mmstick added 8 commits Dec 28, 2017
This resolves a number of significant issues with the way that foreground jobs were being handled. However, let it be known that there is an issue introduced by this commit which causes the piped Ion scripts to fail, due to some outputs getting mixed up. Therefore, be cautious about using Ion for scripts at the moment. I will look into a fix for the issue tomorrow.
Processes within a pipeline should have the same PGID. As this was not
the case, this commit will fix that problem. In addition, processes
within a pipeline should be SIGSTOP'd before they begin execution, and
then SIGCONT'd in the parent when the next job in line is also
SIGSTOP'd. This allows commands earlier in the queue to properly receive
the SIGPIPE signal so that commands like `yes | head` will function
properly.

I've also refactored quite a bit of code within the pipeline execution
module, and have rewritten external command execution to use fork +
execve instead of the Command functionality in the standard library. As
every other piece of functionality in Ion depends upon fork+exec
behavior, it is therefore ideal that external command execution also
works in the same way. This will allow me to do further refactoring in
the near future, and to diagnose some issues that I'm having.

NOTE: Ion remains to be broken with this commit, so I don't recommend to
use Ion until this work is complete.
This is a continuation of the previous commit, and with this change many things will start working again. The usage of `std::process::Command` caused a number of issues that are now resolved with this change.
Ensures that SIGPIPE is handled accordingly within child processes
@jackpot51 jackpot51 self-requested a review Dec 31, 2017
Copy link
Member

left a comment

Great work!

@mmstick mmstick merged commit c2ebfd3 into master Dec 31, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.