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.

Sign up for GitHub

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

Jump to bottom

WIP: ChildProcess #61

Closed
wants to merge 53 commits into from
Closed

WIP: ChildProcess #61

wants to merge 53 commits into from

Conversation

yuya-takeyama
Copy link
Contributor

This is just a proposal.
And no unit-test code is available now.

Please give me advices for...

  • Better API
    • Should the constructor param of Process be Stream object or resource variable?
  • Better naming
    • Factory? Executor?
  • Better implementation
  • ..and so on

Below is the result of example code

$ php examples/child-process.php 
[A] [PID]    pid is 68466
[A] [CMD]    php '-r' 'foreach (range(1, 3) as $i) { echo $i, PHP_EOL; sleep(1); } fputs(STDERR, "Bye.");'
[B] [PID]    pid is 68467
[B] [CMD]    php '-r' 'foreach (range(1, 6) as $i) { echo $i, PHP_EOL; sleep(1); } fputs(STDERR, "Bye.");'
[C] [PID]    pid is 68468
[C] [CMD]    php '-r' 'foreach (range(1, 9) as $i) { echo $i, PHP_EOL; sleep(1); } fputs(STDERR, "Bye.");'
[C] [STDOUT] string(2) "1\n"
[A] [STDOUT] string(2) "1\n"
[B] [STDOUT] string(2) "1\n"
[A] [STDOUT] string(2) "2\n"
[B] [STDOUT] string(2) "2\n"
[C] [STDOUT] string(2) "2\n"
[A] [STDOUT] string(2) "3\n"
[C] [STDOUT] string(2) "3\n"
[B] [STDOUT] string(2) "3\n"
[B] [STDOUT] string(2) "4\n"
[C] [STDOUT] string(2) "4\n"
[A] [STDERR] string(4) "Bye."
[A] [STDOUT] string(0) ""
[A] [STDERR] string(0) ""
[A] [EXIT]   exited with status code 0
[B] [STDOUT] string(2) "5\n"
[C] [STDOUT] string(2) "5\n"
[B] [STDOUT] string(2) "6\n"
[C] [STDOUT] string(2) "6\n"
[B] [STDOUT] string(0) ""
[B] [STDERR] string(4) "Bye."
[B] [STDERR] string(0) ""
[B] [EXIT]   exited with status code 0
[C] [STDOUT] string(2) "7\n"
[C] [STDOUT] string(2) "8\n"
[C] [STDOUT] string(2) "9\n"
[C] [STDERR] string(4) "Bye."
[C] [STDOUT] string(0) ""
[C] [STDERR] string(0) ""
[C] [EXIT]   exited with status code 0

Currently, empty string is emitted when a process is exiting.
It seems because of implementation difference of Stream->handleData() and Connection->handleData().

https://github.com/react-php/react/blob/2a43233661bd148c3eaa9214a89558669d202828/src/React/Socket/Connection.php#L14
https://github.com/react-php/react/blob/2a43233661bd148c3eaa9214a89558669d202828/src/React/Stream/Stream.php#L114

Connection->handleData() closes when hendled empty string and not emits data event then.
But Stream->handleData() emits empty string.

What is the difference for?

@travisbot
Copy link

This pull request passes (merged 161baaa into 2a43233).

igorw
igorw reviewed Aug 18, 2012
View reviewed changes
examples/child-process.php
require __DIR__.'/../vendor/autoload.php';

$loop = new React\EventLoop\StreamSelectLoop();
$factory = new React\ChildProcess\factory($loop);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class name should be uppercase Factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@travisbot
Copy link

This pull request passes (merged e3528ca into 2a43233).

igorw
igorw reviewed Aug 18, 2012
View reviewed changes
src/React/ChildProcess/Factory.php

$cmd = $this->createCommand($file, $args);
$cwd = $options && $options['cwd'] ? $options['cwd'] : getcwd();
$env = $options && $options['env'] ? $options['env'] : $_ENV;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicitly passing the ENV superglobal is a bad idea imo, as it could contain sensitive information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced $_ENV with NULL.

Then the child's $_ENV seems to be empty array.
https://gist.github.com/3386850

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And replaced getcwd() with NULL by same reason.
https://gist.github.com/3386926

@igorw
Copy link
Contributor

igorw commented Aug 18, 2012

Very cool, I like this a lot!

Regarding the empty string issue, I wasn't aware that is also occurs when not in the socket context. Please adjust the stream class to not emit empty strings (and add a test case for it).

Tests for this stuff would be delicious. :)

@travisbot
Copy link

This pull request passes (merged adc2a1c into 2a43233).

@travisbot
Copy link

This pull request passes (merged 29dd20e into 2a43233).

@travisbot
Copy link

This pull request passes (merged 450e093 into 2a43233).

@travisbot
Copy link

This pull request passes (merged 235533c into 2a43233).

@yuya-takeyama
Copy link
Contributor Author

Thank you for reviewing!

I'll send patch for empty string issue as another Pull Request.

And I'll begin writing unit-tests if current API is acceptable.

cboden
cboden reviewed Aug 18, 2012
View reviewed changes
src/React/ChildProcess/Factory.php

public function spawn($file, $args = NULL, $options = NULL)
{
$args = $args ? $args : array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done in the function declaration:

spawn($file, array $args = array(), array $options = null)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.
And all of the NULL was replaced with null.

@travisbot
Copy link

This pull request passes (merged c84cf66 into 2a43233).

@travisbot
Copy link

This pull request fails (merged cc64eb4 into 2a43233).

@igorw
Copy link
Contributor

igorw commented Oct 1, 2012

What's the status on this PR?

@gautamg
Copy link

gautamg commented Oct 15, 2012

This PR would be very useful for my use-case. how can i help get this merged?

@igorw
Copy link
Contributor

igorw commented Oct 15, 2012

Still waiting for a status update from @yuya-takeyama. Especially regarding the failing tests. I would like to see this in core as well. ;-)

@chobie
Copy link

chobie commented Oct 15, 2012

I'll PIN him. maybe he missed github's new notification. :)

@vkartaviy
Copy link

Nice feature! I hope it will be merged soon for further development and improvements.

jmikola
jmikola reviewed Mar 8, 2013
View reviewed changes
tests/React/Tests/ChildProcess/ProcessTest.php
$this->assertNull($capturedExitCodeOfExit);
$this->assertNull($capturedExitCodeOfClose);
$this->assertSame(self::SIGNAL_CODE_SIGTERM, $capturedSignalCodeOfExit);
$this->assertSame(self:: SIGNAL_CODE_SIGTERM, $capturedSignalCodeOfClose);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space after ::?

@csaoh
Copy link

csaoh commented Mar 13, 2013

Would it be possible to have a small overview of what's not working yet in this PR ?

So far I only see one failing test and I think I know why does it fail, but I'm wondering if there are features that haven't been implemented yet or do not have matching tests.

@csaoh csaoh mentioned this pull request Mar 13, 2013
Closed
@igorw
Copy link
Contributor

igorw commented Mar 13, 2013

Closing in favour of #158.

@igorw igorw closed this Mar 13, 2013
@jmikola jmikola mentioned this pull request Mar 21, 2013
Merged
@yuya-takeyama yuya-takeyama deleted the feature/child-process branch December 10, 2013 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@yuya-takeyama @travisbot @igorw @gautamg @chobie @vkartaviy @csaoh @jmikola @cboden