[Process] Unset callback after stop to free memory #16798

Slamdunk · 2015-12-02T09:24:17Z

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

As of yet, a Process instance can't free memory.
The built-in callback has a self-reference to the intance not cleared after instance destruction.
I've tried to change both buildCallback and https://github.com/symfony/process/blob/v2.7.6/Process.php#L1373 usage to avoid self-referencing, but it can't be done without breaking BC, and the memory issue is still there.

A simple test script (that obiously can't be embedded in unit-tests)

for ($inc = 0; $inc < 10000; $inc++) {
    $process = new Symfony\Component\Process\Process('echo 1');
    $process->mustRun();
    $process->stop();
    unset($process);

    echo (memory_get_usage(true) / 1000000) . ' MB ' . $inc . PHP_EOL;
}

Before:

1.572864 MB 0
5.505024 MB 9999

After:

1.572864 MB 0
1.572864 MB 9999

Ok, in this simple scenario 4 MB of RAM is nothing, but in our business app full of IOC where unset-ting is hard, our RAM explodes.

After stop(), the callback is no longer necessary.

To be clear, the garbage collector in the previous example was already active.
Deactivating it manually, which somewhere is needed to avoid particluar segfaults, obiously leads to worse scenarios:

Before:

 gc_disable();
 1.572864 MB 0
29.360128 MB 9999

After:

 gc_disable();
 1.572864 MB 0
 1.572864 MB 9999

stof · 2015-12-02T11:09:09Z

src/Symfony/Component/Process/Process.php

@@ -801,6 +801,8 @@ public function stop($timeout = 10, $signal = null)
            $this->close();
        }

+        $this->callback = null;


Please add a comment explaining why it is done

stof · 2015-12-02T11:10:04Z

this should also be reset at the end of a normal run (i.e. without the user stopping the process), as the callback won't be necessary anymore either (basically, the same benchmark without the call to ->stop())

stof · 2015-12-02T11:15:25Z

And I think this can break the call to getOutput or getErrorOutput in the current implementation

Slamdunk · 2015-12-02T13:48:09Z

@stof unsetting the callback automatically after run breaks getOutput and getErrorOutput, that's why I put it in stop, a method called only on __destruct or manually in particular cases.

I am sure that it does not break anything where I put it because:

No test fails (the one failing in Windows is unreleated)
The stop calls updateStatus, which calls readPipes and so all the Pipes are already filled. After that, every internal call to readPipes will result in an empty foreach, unless you restart the process with another callback build

My tests on unsetting the callback in any other context other then stop, like __destruct, has led to ineffective memory freeing, or breaking something.

nicolas-grekas · 2015-12-03T11:37:56Z

I think the callback should also be reset in wait (and thus in run also since wait is used in run).

Slamdunk · 2015-12-03T11:43:12Z

@nicolas-grekas only after pipes are close you can be sure to unset the callback, and the pipes are closed only in stop: https://github.com/symfony/process/blob/v3.0.0/Process.php#L1356

nicolas-grekas · 2015-12-03T11:47:58Z

The line you linked doesn't proves anything about what stop does that wait doesn't do. And in fact, as far as I read correctly, wait waits for isRunning to be false, which means pipes are closed. Or did I read too quickly?

Slamdunk · 2015-12-03T13:21:28Z

@nicolas-grekas your assumptions are based on the ability of PipesInterface::readAndWrite $close flag to close the Pipes. What I read in the interface (https://github.com/symfony/process/blob/v3.0.0/Pipes/PipesInterface.php#L43) is "Whether to close pipes if they've reached EOF" which means is not mandatory to close the pipes with the flag activated.
In fact on Windows, the pipes may not be closed even with the flag set to true: https://github.com/symfony/process/blob/v3.0.0/Pipes/WindowsPipes.php#L132 .
The only way to be sure pipes are closed, is to call PipesInterface::close, which you can be sure is always called if and only if the Process::stop is called because of this and this together.

nicolas-grekas · 2015-12-03T14:05:51Z

@Slamdunk let me give you my understanding of the code:

wait loops until the process is running
isRunning returning false means that it has called Process->close
which means that processPipes are fclosed

readPipes is not part of this closing procedure (it happens earlier in the call chain)

nicolas-grekas · 2015-12-03T14:08:57Z

Btw, this means that resetting the callback should be done only in the close method!

Slamdunk · 2015-12-03T15:38:57Z

@nicolas-grekas 👍

nicolas-grekas · 2015-12-03T16:39:43Z

👍 for merging on 2.3

stof · 2015-12-05T10:15:17Z

👍

stof · 2015-12-05T10:16:30Z

Thank you @Slamdunk.

…dunk) This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #16798). Discussion ---------- [Process] Unset callback after stop to free memory | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As of yet, a `Process` instance can't free memory. The built-in callback has a self-reference to the intance not cleared after instance destruction. I've tried to change both `buildCallback` and https://github.com/symfony/process/blob/v2.7.6/Process.php#L1373 usage to avoid self-referencing, but it can't be done without breaking BC, and the memory issue is still there. A simple test script (that obiously can't be embedded in unit-tests) ```php for ($inc = 0; $inc < 10000; $inc++) { $process = new Symfony\Component\Process\Process('echo 1'); $process->mustRun(); $process->stop(); unset($process); echo (memory_get_usage(true) / 1000000) . ' MB ' . $inc . PHP_EOL; } ``` Before: ``` 1.572864 MB 0 5.505024 MB 9999 ``` After: ``` 1.572864 MB 0 1.572864 MB 9999 ``` Ok, in this simple scenario 4 MB of RAM is nothing, but in our business app full of IOC where `unset`-ting is hard, our RAM explodes. After `stop()`, the callback is no longer necessary. To be clear, the garbage collector in the previous example was already active. Deactivating it manually, which somewhere is needed to avoid particluar segfaults, obiously leads to worse scenarios: Before: ``` gc_disable(); 1.572864 MB 0 29.360128 MB 9999 ``` After: ``` gc_disable(); 1.572864 MB 0 1.572864 MB 9999 ``` Commits ------- ec93b9a [Process] Unset callback after stop to free memory

[Process] Unset callback after stop to free memory

7f1b008

carsonbot added the Status: Needs Review label Dec 2, 2015

Slamdunk mentioned this pull request Dec 2, 2015

Unset callback after stop symfony/process#12

Closed

stof reviewed Dec 2, 2015
View reviewed changes

[Process] Commented unset of callback

d357245

[Process] Callback unset when we are sure pipes are closed

0cfb41b

stof closed this Dec 5, 2015

Slamdunk deleted the process/unset-callback branch December 7, 2015 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Process] Unset callback after stop to free memory #16798

[Process] Unset callback after stop to free memory #16798

Uh oh!

Slamdunk commented Dec 2, 2015

Uh oh!

stof Dec 2, 2015

Uh oh!

stof commented Dec 2, 2015

Uh oh!

stof commented Dec 2, 2015

Uh oh!

Slamdunk commented Dec 2, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

stof commented Dec 5, 2015

Uh oh!

stof commented Dec 5, 2015

Uh oh!

Uh oh!

Uh oh!

[Process] Unset callback after stop to free memory #16798

[Process] Unset callback after stop to free memory #16798

Uh oh!

Conversation

Slamdunk commented Dec 2, 2015

Uh oh!

stof Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

stof commented Dec 2, 2015

Uh oh!

stof commented Dec 2, 2015

Uh oh!

Slamdunk commented Dec 2, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

Slamdunk commented Dec 3, 2015

Uh oh!

nicolas-grekas commented Dec 3, 2015

Uh oh!

stof commented Dec 5, 2015

Uh oh!

stof commented Dec 5, 2015

Uh oh!

Uh oh!