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

feat(cli): Enable sub-process to run in detached mode #5501 #5836

Closed
wants to merge 2 commits into from

Conversation

manikawnth
Copy link

The PR is to enable processes started via Deno.run() to run in detached mode.

Prior art is detailed in: #5501 (comment)

Since the detached process is standard across *nix and windows platforms, node's way of run options is chosen.

@CLAassistant
Copy link

CLAassistant commented May 25, 2020

CLA assistant check
All committers have signed the CLA.

@ry
Copy link
Member

ry commented May 25, 2020

Very cool! I need to research this a bit, but the proposed API seems reasonable at first glance.

Have you given some thought as to how this could be tested? It seems we probably need a custom test in cli/tests/integration_tests.rs...

@manikawnth
Copy link
Author

manikawnth commented May 25, 2020

@ry

Have you given some thought as to how this could be tested? It seems we probably need a custom test in cli/tests/integration_tests.rs...

Actually we can do this in unit -test process_test.ts, with some setTimeout hacks. Granparent spawns parent in attached mode. Parent spawns child in detached mode. After a brief timeout, parent exits. Grandparent (our assertion) will compare the the ppid of the grand child before and after.

Here's the code,

unitTest(
  { perms: { read: true, run: true, write: true } }, 
  async function runDetached(): Promise<void> {
    const enc = new TextEncoder();
    const cwd = await makeTempDir({ prefix: "deno_detach_test" });
    const denoParent = `
function start() {
  let p = Deno.run({
    cmd:['sleep', '120'],
    detached: true,   
    stdin: "null",
    stdout: "null",
    stderr: "null"
  })  

  let child_pid_buf = new TextEncoder().encode(p.pid.toString())            
  Deno.stdout.writeSync(child_pid_buf);
  //to make sure pipe stays alive till we consume the data
  setTimeout(() => {
    Deno.exit();  //Spawner exits  
  }, 300);  
}
  
start();
`;

    Deno.writeFileSync(`${cwd}/parent.ts`, enc.encode(denoParent));

    let p = run({
      cmd:['deno', 'run', '--allow-run','parent.ts'],
      cwd,
      detached: false,
      stdin: "piped",
      stdout: "piped",
      stderr: "piped"
    })  
  
    const buf = new Uint8Array(20);
    let c_out_len = await p.stdout!.read(buf);
    if (c_out_len == null) {
      throw new Error("p.stdout.read(...) should not be null");
    }
    let gc_pid = new TextDecoder().decode(buf.subarray(0, c_out_len));
    console.log(`gc_pid: ${gc_pid}`);
    
    //read the old stat file
    const old_file = await Deno.open(`/proc/${gc_pid}/stat`, {read: true});
    let old_stat: string = new TextDecoder().decode(Deno.readAllSync(old_file));

    //artificial wait
    function delay(ms: number) {
      return new Promise(resolve => setTimeout(resolve, ms));
    }
    await delay(3000);

    //read the new stat file
    const new_file = await Deno.open(`/proc/${gc_pid}/stat`, {read: true});
    let new_stat: string = new TextDecoder().decode(Deno.readAllSync(new_file));
  
    let old_pid = old_stat.split(' ')[0];      
    let old_ppid = old_stat.split(' ')[3];
    console.log(`old_pid: ${old_pid}`);
    console.log(`old_ppid: ${old_ppid}`);

    let new_pid = new_stat.split(' ')[0];      
    let new_ppid = new_stat.split(' ')[3];
    console.log(`new_pid: ${new_pid}`);
    console.log(`new_ppid: ${new_ppid}`);

    const status = await p.status();
    assertEquals(old_pid, new_pid);
    assertNotEquals(old_ppid, new_ppid);
    old_file.close();
    new_file.close();
    p.stdin!.close();
    p.stdout!.close();
    p.stderr!.close();
    p.close();
  }
);

Unit tests have passed in my ubuntu. I'll PR this, if you're okay. And ofcourse, only if you're okay to ignore this test, for windows.

@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS labels May 26, 2020
@ry
Copy link
Member

ry commented Jun 10, 2020

@manikawnth I'm very sorry for the slow response.

I think for this feature, it probably is better to write a custom integration test in cli/tests/integration_tests.rs where you manually spawn a deno process - which maybe prints the PID of the subprocess to stdout. Then in Rust we can do checks to see that it stays alive...

@bartlomieju bartlomieju added this to the 1.3.0 milestone Jul 14, 2020
@bartlomieju bartlomieju removed this from the 1.3.0 milestone Aug 12, 2020
@bartlomieju
Copy link
Member

@manikawnth please sign CLA, otherwise we won't be able to land this PR

@bartlomieju
Copy link
Member

Superseded by #7481

@bartlomieju bartlomieju closed this Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants