[Process] fixed fatal errors in getOutput and getErrorOutput when process was not started #9452

max-voloshyn · 2013-11-05T22:36:03Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#10022
License	MIT
Doc PR

Added logic exceptions for prevent fatal errors in getOutput and getErrorOutput when process was not started.

Fatal error: Call to a member function readAndCloseHandles() on a non-object in /home/max/symfony/src/Symfony/Component/Process/Process.php on line 1117

Call Stack:
    0.0002     231816   1. {main}() /home/max/phpunit/composer/bin/phpunit:0
    0.0038     526688   2. PHPUnit_TextUI_Command::main() /home/max/phpunit/composer/bin/phpunit:63
    0.0038     526912   3. PHPUnit_TextUI_Command->run() /home/max/phpunit/PHPUnit/TextUI/Command.php:126
    0.2261    4022696   4. PHPUnit_TextUI_TestRunner->doRun() /home/max/phpunit/PHPUnit/TextUI/Command.php:173
    0.2302    4353544   5. PHPUnit_Framework_TestSuite->run() /home/max/phpunit/PHPUnit/TextUI/TestRunner.php:415
    0.3218    5675504   6. PHPUnit_Framework_TestSuite->run() /home/max/phpunit/PHPUnit/Framework/TestSuite.php:729
   15.1171   38257632   7. PHPUnit_Framework_TestCase->run() /home/max/phpunit/PHPUnit/Framework/TestSuite.php:729
   15.1172   38257632   8. PHPUnit_Framework_TestResult->run() /home/max/phpunit/PHPUnit/Framework/TestCase.php:782
   15.1173   38258544   9. PHPUnit_Framework_TestCase->runBare() /home/max/phpunit/PHPUnit/Framework/TestResult.php:661
   15.1174   38275544  10. PHPUnit_Framework_TestCase->runTest() /home/max/phpunit/PHPUnit/Framework/TestCase.php:839
   15.1175   38276408  11. ReflectionMethod->invokeArgs() /home/max/phpunit/PHPUnit/Framework/TestCase.php:987
   15.1175   38276440  12. Symfony\Component\Process\Tests\AbstractProcessTest->testGetOutputProcessNotRunning() /home/max/phpunit/PHPUnit/Framework/TestCase.php:987
   15.1175   38277728  13. Symfony\Component\Process\Process->getOutput() /home/max/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:636
   15.1175   38277824  14. Symfony\Component\Process\Process->readPipes() /home/max/symfony/src/Symfony/Component/Process/Process.php:383

fix code after review by @Tobion
fix code after review by @jakzal
fix code after merging [Process] Fix process status in TTY mode #10412 (@romainneutron's notice)

Tobion · 2013-11-06T09:08:25Z

src/Symfony/Component/Process/Process.php

     * @return string The process output
     *
     * @api
     */
    public function getOutput()
    {
+        if (!$this->isStarted()) {
+            throw new LogicException(sprintf('%s does not make sense if command is not started', __FUNCTION__));


Process must be started before calling __FUNCTION__

Edited Command -> Process

max-voloshyn · 2013-11-06T10:27:56Z

@Tobion thanks, code will be fixed

piotrpasich · 2013-11-06T11:42:58Z

src/Symfony/Component/Process/Process.php

@@ -401,12 +406,17 @@ public function getIncrementalOutput()
    /**
     * Returns the current error output of the process (STDERR).
     *
+     * @throws Exception\LogicException


As @Tobion said - throws after return with empty line between. You can find an example in here http://symfony.com/doc/current/contributing/code/standards.html

max-voloshyn · 2013-11-06T18:51:14Z

@Tobion, @piotrpasich, thanks!
Fixed and squashed.

Tobion · 2013-11-06T20:47:13Z

Looking at the process code, it looks to me that there will also be PHP errors when calling some other methods before starting the process. For example hasBeenStopped will access $this->processInformation['stopped'] which IMO is not set when the process is not started. Would be a similar problem to the one you fixed here. Maybe you can have a look at them as well?

max-voloshyn · 2013-11-08T08:59:35Z

OK, I will look at them on weekend.

max-voloshyn · 2013-11-23T11:38:28Z

I have noticed that many similar problems are beginning in updateStatus method when we do:

if (self::STATUS_STARTED !== $this->status) {
    return;
}

Maybe it would be better to throw logic exception instead of simple return, because we really can't update status of not started process.

Thus we can don't throw logic exceptions in all dependent methods.
Only disadvantage I see here is less verbose of exception message.
But on the other hand it is better than we are having now.

@Tobion, what you think about it?

max-voloshyn · 2013-11-26T08:11:16Z

Another opinions will come in handy :)

wouterj · 2013-11-26T08:19:52Z

src/Symfony/Component/Process/Process.php

@@ -371,10 +371,16 @@ public function signal($signal)
     *
     * @return string The process output
     *
+     * @throws Exception\LogicException In case the process is not started


Exception\LogicException does not correspond with the class used in the code (LogicException) on of those should be changed (same below)

ping @max-voloshin

jakzal · 2013-12-27T23:26:05Z

@max-voloshin do you have plans to finish this PR?

max-voloshyn · 2013-12-28T12:34:54Z

@jakzal , finish:)

Similar problems can be fixed in another PR, if anyone has enough experience for this.
I have some thoughts: #9452 (comment), but I am not sure.. So I left it as is.

…cess was not started

fabpot · 2014-01-17T13:40:05Z

@romainneutron Can you check this PR please?

romainneutron · 2014-01-17T13:48:28Z

This looks good to me.

@max-voloshin FYI the updateStatus method might be renamed updateStatusIfRunning. This method should not throw an exception otherwise you would broke a lot of the process logic

romainneutron · 2014-01-17T13:51:07Z

Humm, wait.

The check on $this->isStarted() might be wrong. $this->isStarted() would return false in case the process is terminated (once it has stop).

I'm doing some tests right now

romainneutron · 2014-01-17T13:55:19Z

this piece of code should fail (and actually does not) :

$process = new Process('php -m');
$process->run();
$process->wait();

max-voloshyn · 2014-01-17T13:59:08Z

@romainneutron , thanks! I will add tests for this and improve code.

romainneutron · 2014-01-17T13:59:27Z

src/Symfony/Component/Process/Process.php

     */
    public function wait($callback = null)
    {
+        if (!$this->isStarted()) {


this should be if (!$this->isStarted() || $this->status === self::STATUS_TERMINATED)

@romainneutron, with this change I break testTTYCommand, because in start method we have

if ($this->tty) { $this->status = self::STATUS_TERMINATED; return; }

:(
I've to admit the TTY mode is something I don't know very well. I'm gonna dig this tonight

Hello and sorry for the late reply,

I've justed submitted #10412 that should make this change doable and your PR mergeable

@romainneutron, thanks! I will wait for merging your PR and then rebase my branch with 2.3

fabpot · 2014-03-10T19:58:45Z

#10412 has been merged now.

Tobion · 2014-03-10T20:37:52Z

My remark #9452 (comment) at is still open and I think we should not merge this before finding a general solution.

@max-voloshin I basically agree with #9452 (comment) but it doesnt work universally. In getStatus for example it should not throw an exception when calling updateStatus. So one has to be careful to apply it in class.

romainneutron · 2014-03-12T09:50:01Z

Hello @Tobion, thanks for your comment.
It actually does not produce any PHP error as long as the property is null.

The following code:

error_reporting(-1);
ini_set('display_errors', 'on');
$a = null;
var_dump($a['key']);

produces :

NULL

whereas (note that $a is now an array)

error_reporting(-1);
ini_set('display_errors', 'on');
$a = array();
var_dump($a['key']);

produces :

PHP Notice:  Undefined index: key in /tmp/test.php on line 4
PHP Stack trace:
PHP   1. {main}() /tmp/test.php:0

So the following code should not produce any error:

$process = new Process('php -m');
$process->getTermSignal();

romainneutron · 2014-03-12T10:40:51Z

I've done a review of the methods and here's what could be fixed:

Process::getExitCode, Process::getExitCodeText and Process::isSuccessful return null when the process has not started yet, it should throw an exception as well as for getErrorOutput.

What about other methods that point to the final status of the process such as Process::getTermSignal, Process::hasBeenStopped, etc...? I think we should do the same as for the methods above and what you did.

checkTimeout throws a timeout exception when the process is not started yet or already terminated, it could be fixed with:

    public function checkTimeout()
    {
        if ($this->status !== self::STATUS_STARTED) {
            return;
        }
        // ...
    }

Tobion · 2014-03-12T16:26:59Z

@romainneutron Thanks for the information. I didn't know that. One can learn php inconsistencies even after many years..

But even when it does not throw an error, the methods will not return the expected values. So hasBeenSignaled can also return null at the moment, instead of a Boolean as documented.

romainneutron · 2014-03-12T16:30:43Z

right, do you think we should cast to a boolean or throw an exception in case the process has not run ?
I think we should throw an exception to be consistent with other methods.

Tobion · 2014-03-12T16:36:25Z

I think we should throw an exception to be consistent with other methods.

Agreed

fabpot · 2014-03-18T17:57:24Z

Closing in favor of #10480

…ut when process was not started (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10022 | License | MIT This PR replaces #9452 and address the latest changes. Side note : I've not updated `getExitCode`, `getExitCodeText` and `isSuccessful` as they were explicitly tested to return null in case the process is not started or terminated. I think it would be a BC break to do that. Commits ------- 449fe01 [Process] Trow exceptions in case a Process method is supposed to be called after termination 0ae6858 [Process] fixed fatal errors in getOutput and getErrorOutput when process was not started

Tobion reviewed Nov 6, 2013
View reviewed changes

piotrpasich reviewed Nov 6, 2013
View reviewed changes

wouterj reviewed Nov 26, 2013
View reviewed changes

max-voloshyn closed this Dec 28, 2013

max-voloshyn deleted the fix_fatals branch December 28, 2013 12:11

max-voloshyn reopened this Dec 28, 2013

jakzal mentioned this pull request Jan 14, 2014

[Process] Fatal error when calling wait before run #10022

Closed

max-voloshyn added 2 commits January 16, 2014 22:43

[Process] fixed fatal errors in getOutput and getErrorOutput when pro…

3fa4899

…cess was not started

[Process] fixed symfony#10022

fb641c8

romainneutron reviewed Jan 17, 2014
View reviewed changes

romainneutron mentioned this pull request Mar 18, 2014

[Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started #10480

Merged

fabpot closed this Mar 18, 2014

Uh oh!

[Process] fixed fatal errors in getOutput and getErrorOutput when process was not started #9452

[Process] fixed fatal errors in getOutput and getErrorOutput when process was not started #9452

Uh oh!

Conversation

max-voloshyn commented Nov 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-voloshyn commented Nov 6, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-voloshyn commented Nov 6, 2013

Uh oh!

Tobion commented Nov 6, 2013

Uh oh!

max-voloshyn commented Nov 8, 2013

Uh oh!

max-voloshyn commented Nov 23, 2013

Uh oh!

max-voloshyn commented Nov 26, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakzal commented Dec 27, 2013

Uh oh!

max-voloshyn commented Dec 28, 2013

Uh oh!

fabpot commented Jan 17, 2014

Uh oh!

romainneutron commented Jan 17, 2014

Uh oh!

romainneutron commented Jan 17, 2014

Uh oh!

romainneutron commented Jan 17, 2014

Uh oh!

max-voloshyn commented Jan 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Mar 10, 2014

Uh oh!

Tobion commented Mar 10, 2014

Uh oh!

romainneutron commented Mar 12, 2014

Uh oh!

romainneutron commented Mar 12, 2014

Uh oh!

Tobion commented Mar 12, 2014

Uh oh!

romainneutron commented Mar 12, 2014

Uh oh!

Tobion commented Mar 12, 2014

Uh oh!

fabpot commented Mar 18, 2014

Uh oh!

Uh oh!