reactphp / child-process

Event-driven library for executing child processes with ReactPHP.
https://reactphp.org/child-process/
MIT License
306 stars 45 forks source link

Clean up API #72

Open clue opened 5 years ago

clue commented 5 years ago

The roadmap (#31) currently mentions "clean up API" for the v0.7.0 release, so I'm filing this ticket to discuss possible things that could be cleaned up.

Here's some of the things that I would like to address:

1.) The Process constructor and the start() method form a temporal dependence (you have to call start() exactly once after constructing). Additionally, because creating a process requires creating an instance of Process means that this is really hard to test in higher-level components that dynamically create the command for example (you can't inject a mock for instance, see https://github.com/clue/reactphp-ssh-proxy/blob/7ac2c246a1a5fd86246452d04346092cdd8bb1a7/src/SshProcessConnector.php#L89 for example). As an alternative, I would suggest adding a Launcher (name subject to discussion) which acts as a "factory" to create processes. This solves many of these problems, allows easily creating custom implementations for the points discussed below and also allows simply injecting a mock to higher-level implementations. Example usage:

// concept only
$launcher = new Launcher($loop);
$process = $launcher->launch('ping google.com');

$process->on('exit', 'var_dump');

2.) Inherited file descriptors (#51) are likely not what most use cases want. We should provide a "default launcher" which automatically takes care of closing all file descriptors. This can easily be integrated into the "launcher" concept above and we may or may not want to provide additional options to control this behavior.

3.) Windows support (#67) requires special care even for the easiest use cases. We should provide a "windows launcher" which automatically takes care of setting up I/O redirection for most common use cases. This can easily be integrated into the "launcher" concept above.

4.) The API is quite complex for common use cases. We should provide additional helper methods to cover common use cases. This can easily be integrated into the "launcher" concept above. For example, it's very common you only consume readable data from process output or want to buffer all the data:

// concept only
$launcher = new Launcher($loop);
$stream = $launcher->stream('ping example.com');

$stream->on('data', function ($chunk) {
    echo $chunk;
});

// concept only
$launcher = new Launcher($loop);
$promise = $launcher->buffer('ping example.com');

$promise->then(function ($data) {
    echo $data;
});

Any input is welcome! :+1:

loilo commented 5 years ago

Having a Promise-based helper as in (4) would be really useful.

A thing that should be baked in: Such an interface would offer the great opportunity of combining $promise->cancel() with $process->terminate() for really easy premature elimination of long-running processes.

ghost commented 5 years ago

The only issue with 1) would be that there is no interface to type declare against. With #74 in mind an interface would possibly be required anyway, so the problem of mocking would be solved. To be clear, the "low level" API of directly using a Process should still exist, even when a launcher should be added.

However interfaces should exist so that the implementation can be arbitrarily replaced or customized, without introducing any dependencies or adaptions in code.

ghost commented 4 years ago

I'd like to pick up 1) + 4) (Launcher concept), are there any specific helper methods that need to exist?

On my list currently: