Page MenuHomePhabricator

ExecFuture->resolveKill() should check if $this->proc is null before calling proc_terminate.
Closed, ResolvedPublic

Description

It seems like the right thing is to surround proc_terminate(...) in an if ($this->proc). I'm not sure, though. I can provide more context if this isn't obviously the right thing to do.

Event Timeline

cakoose raised the priority of this task from to Needs Triage.
cakoose updated the task description. (Show Details)
cakoose added a project: libphutil.
cakoose added a subscriber: cakoose.

More context would be helpful. How can proc be null?

You could do something like this:

$future = new ExecFuture('...');
$future->resolveKill();

However, this seems meaningless and should probably (?) raise an exception, not silently succeed. Is there a reason it might not be meaningless?

This came up with a custom Arcanist linter we have (I didn't write it, but I'm now trying to fix the exception). The lintPath() function looks like this:

public function lintPath($path) {
    if (is_null($this->linterProcess)) {
        $this->linterProcess = new ExecFuture(...);
        $this->write("", true);
        $this->start();
    }
    
    $this->linterProcess->write($path . "\n", true);

    $done = false;
    $lint_output = '';
    while (!$done) {
        usleep(50000);
        list($stdout, $stderr) = $this->linterProcess->read();
        if ($stderr !== '') {
            list($stdout_more, $stderr_more) = $this->linterProcess->resolveKill();
            $this->linterProcess = null;
            $stdout .= $stdout_more;
            $done = true;
        }
        $line_output .= $stdout;
        if (substr($lint_output, -5) == "DONE\n") {
            $done = true;
        }
    }
}

Is this the wrong pattern for doing what we want?

Even just having a more descriptive exception would help, since then I'd try and figure out how to fix it on our side instead of bugging you :-)

Ah, okay. I think the issue is that the future is already resolved. That is, your code does this, in effect:

$future->resolve();
$future->resolveKill();

We should be robust against that, since calling resolve() or resolvex() twice does work.

This linter is built kind of oddly, and the construction here is not ideal. In particular:

  • We can't do any I/O during a usleep(50000), so the process can read at most 4096 bytes every 50ms, i.e. the maximum bandwidth is 80KB/sec. This might be OK, but I could imagine it being a bottleneck if you were linting a whole codebase. A better call is $future->resolve(0.05), which will also return early if the process exits.
  • The resolve*() methods don't use the same cursor that read() does -- check the documentation for read(). The reason things work this way is that it makes it much easier to, e.g., echo status messages from stderr to the console as a process runs, and then do something useful with the output when process exits. So $stdout_more and $stderr_more will have the entire process output. You need to call discardBuffers() after read() (or read() after resolveKill()) to avoid this.

The "most correct" way to implement something like this is to use PhutilProtocolChannel. This is probably overkill and more work than you want to do, though.

Finally, unless the linter is very very expensive to start up or very unusual, this approach is much slower than using the parallel/futures approach other linters use: although it doesn't need to keep re-launching a process, it can only lint one file at a time. If you use a more standard pattern (one process per file, with parallelism) and the linter isn't hugely heavy, performance will probably improve by a large factor. You would do this by extending ArcanistFutureLinter and having buildFutures() do something like this:

$futures = array();
foreach ($paths as $path) {
  $futures[$path] = id(new ExecFuture('...'))
    ->write($path); // No ", true" -- close the pipe.
}

I think my guess was right. Basically, the program is doing this now:

$future->read(); // Internally, the future resolves here, maybe by exiting with an error.
// ...
$future->resolveKill(); // We explode here because resolveKill() is dumb and the future is already resolved.

After D7688, the resolveKill() should behave more reasonably (it will just return the results of the already-resolved process).

Oh, the output of resolveKill() is also:

list($err, $stdout, $stderr) = $future->resolveKill();

...not just the latter two, as written.

epriestley triaged this task as Normal priority.