Rewrite std::run

[catamorphism]

Original title: "Rewrite core::run"

As per a FIXME, // Spawn two entire schedulers to read both stdout and sterr // in parallel so we don't deadlock while blocking on one // or the other. FIXME: Surely there's a much more clever way // to do this.

[pcwalton]

core::run just plain needs to be rewritten.

[Dretch]

Here are some things I think are issues with core::run:

• There are 4 different API calls for starting child processes, and the most flexible call returns a process id instead of a Program value:

  pub fn spawn_process(prog: &str, args: &[~str],
                       env: &Option<~[(~str,~str)]>,
                       dir: &Option<~str>,
                       in_fd: c_int, out_fd: c_int, err_fd: c_int) -> pid_t { ... }
  pub fn run_program(prog: &str, args: &[~str]) -> int { ... }
  pub fn start_program(prog: &str, args: &[~str]) -> Program { ... }
  pub fn program_output(prog: &str, args: &[~str]) -> ProgramOutput { ... }
  

This is awkward because if (for example) you have been using program_output but then decide you need to change the current directory of the subprocess, you need to switch to using spawn_process and duplicate a bunch of code from core::run in order to read everything from stdout/stderr. This issue has lead to duplicate code in librustc/markdown_writer.rs and compiletest/procsrv.rs.

Ideally I think there would be just one API call to start a process, and that call would return a Program.

• core::run.Program.finish returns the exit code of the process... but when called again it returns 0. I think it would make more sense for the exit code to be stored from the first call and returned from all subsequent calls. The documentation should also describe the behaviour.
• On Unix platforms, there are race conditions that can happen if a user gets the process id out of a Program and calls run::waitpid themselves - because the Program will then call run::waitpid again... but the first call to run::waitpid will have freed up the process id and it might now belong to a different process. I think that perhaps run::waitpid should be removed from the public API to fix this issue.
• There is a similar race condition in core::run.Program.force_destroy / destroy ... it might end up killing a different process due to a re-used process id.
• core::run::program_output returns the output of stdout/stderr as ~str values, which assumes the output is utf-8, which doesn't seem like a safe assumption.
• The documentation should make clear that the Program destructor will wait for the process to finish.

Does anyone else feel that these are issues? Should they be solved in ways other than I suggested? If there are no problems with the above then I will try to submit some pull requests.

[Dretch]

Whilst preparing some fixes/refactoring I found another issue:

One can keep reading from a Program's output @Reader even when the Program has been destroyed, and this means one is reading from an invalid *libc::FILE.

I.e.:

use core::run;

fn main() {
    let output;
    {
        let mut p = run::Process::new("cat", [], run::ProcessOptions::new());
        output = p.output();
    }
    output.read_byte();
}

Produces, when run:

*** glibc detected *** ./test-run-input-after-finish~: corrupted double-linked list: 0x00007f6c38201d10 ***

[brson]

cc #6436

[pnkfelix]

�Visiting for triage, email from 2013 sep 23.
�
Some of the items above have been done (and merged in) with SHA: b7294e1. But from the commit message there, it sounds like some (unspecified) things remain.

So I'm going to attempt to build a checklist here of the items listed above, so we can determine if this ticket is complete.

• replace four API calls with one general function (and maybe some simplifying wrappers?): done, run::Process::new
• race condition on Unix between user getting process id and calling waitpid and Process itself calling waitpid, due to process id reuse; done? -- waitpid is private, does that suffice?
• perhaps address above by removing waitpid
• race condition between force_destroy and destroy due to process id reuse.
• run::process_output returns output as ~str, which assumes utf-8, which is an unsafe assumptions.
• documentation should say that destructor waits for program to terminate.