[Console][Process] Free process resources #44882

jtojnar · 2021-12-31T23:26:45Z

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

I am experiencing Failed to open stream: Too many open files error when running tests. Since lsof shows new /dev/pts/1 being added every second, I tried closing the open processes and also squizlabs/PHP_CodeSniffer#3523 but to no avail. But it is a good practice nonetheless.

carsonbot · 2021-12-31T23:26:49Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

nicolas-grekas · 2022-01-01T12:14:59Z

You say that this doesn't fix anything for you, isn't it?
Closing resources explicitly is not needed so I'm not sure there's any reason to do this chance.

jtojnar · 2022-01-01T12:18:44Z

Cannot say it does not fix anything but it is not sufficient to fix the issue I am experiencing.

jtojnar · 2022-01-01T12:20:08Z

I am going by the docs, which say “[return value] should be freed using proc_close() when you are finished with it”

adrienfr · 2022-01-03T09:37:26Z

Hi,

Same here, since upgrading to SF 5.3.13, my PHPUnit test suite fails with :

PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

Testing 
.............................................................   61 / 4733 (  1%)
.............................................................  122 / 4733 (  2%)
.............................................................  183 / 4733 (  3%)
.............................................................  244 / 4733 (  5%)
.............................................................  305 / 4733 (  6%)
.............................................................  366 / 4733 (  7%)
.............................................................  427 / 4733 (  9%)
.............................................................  488 / 4733 ( 10%)
.............................................................  549 / 4733 ( 11%)
.............................................................  610 / 4733 ( 12%)
.............................................................  671 / 4733 ( 14%)
.............................................................  732 / 4733 ( 15%)
................PHP Warning:  include(/var/www/project/symfony/vendor/phpunit/phpunit/src/Framework/Error/Warning.php): failed to open stream: Too many open files in /var/www/project/symfony/vendor/symfony/error-handler/DebugClassLoader.php on line 349
PHP Warning:  include(): Failed opening '/var/www/project/symfony/vendor/composer/../phpunit/phpunit/src/Framework/Error/Warning.php' for inclusion (include_path='.:/usr/share/php:/var/www/project/symfony/vendor/deployer/recipes') in /var/www/project/symfony/vendor/symfony/error-handler/DebugClassLoader.php on line 349
PHP Warning:  include(/var/www/project/symfony/vendor/phpunit/phpunit/src/Framework/Error/Warning.php): failed to open stream: Too many open files in /var/www/project/symfony/vendor/composer/ClassLoader.php on line 571
PHP Warning:  include(): Failed opening '/var/www/project/symfony/vendor/composer/../phpunit/phpunit/src/Framework/Error/Warning.php' for inclusion (include_path='.:/usr/share/php:/var/www/project/symfony/vendor/deployer/recipes') in /var/www/project/symfony/vendor/composer/ClassLoader.php on line 571
Class 'PHPUnit\Framework\Error\Warning' not found

I'll try to set up a reproducer

jtojnar · 2022-01-03T09:46:25Z

@adrienfr do you know the previous version of dependencies? Also I am not completely sure Symfony is responsible, might also be PHPUnit or something else.

adrienfr · 2022-01-03T10:34:22Z

@adrienfr do you know the previous version of dependencies? Also I am not completely sure Symfony is responsible, might also be PHPUnit or something else.

I can confirm this is SF related, no more issues when downgrading to previous version. I also tested on SF 5.4.2, the issue is also here.

Downgrading symfony/string (v5.3.13 => v5.3.10): Extracting archive
Downgrading symfony/console (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/property-info (v5.3.13 => v5.3.8): Extracting archive
Downgrading symfony/property-access (v5.3.13 => v5.3.8): Extracting archive
Downgrading symfony/http-foundation (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/dom-crawler (v5.3.13 => v5.3.7): Extracting archive
Downgrading symfony/config (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/yaml (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/translation (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/dependency-injection (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/process (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/var-dumper (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/error-handler (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/http-kernel (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/doctrine-bridge (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/finder (v5.3.13 => v5.3.7): Extracting archive
Downgrading symfony/cache (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/framework-bundle (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/validator (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/proxy-manager-bridge (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/templating (v5.3.13 => v5.3.7): Extracting archive
Downgrading symfony/security-core (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/security-http (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/security-bundle (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/form (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/http-client (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/intl (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/twig-bridge (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/debug-bundle (v5.3.13 => v5.3.4): Extracting archive
Downgrading symfony/dotenv (v5.3.13 => v5.3.10): Extracting archive
Downgrading symfony/mime (v5.3.13 => v5.3.11): Extracting archive
Downgrading symfony/mailer (v5.3.13 => v5.3.9): Extracting archive
Downgrading symfony/serializer (v5.3.13 => v5.3.12): Extracting archive
Downgrading symfony/web-profiler-bundle (v5.3.13 => v5.3.8): Extracting archive

=> OK (4733 tests, 31650 assertions)

Might not be able to provide a reproducer thus, got 4733 tests and it fails at 14-15%. Do you have any idea how to process?

Should I create a dedicated issue on GitHub with the error message failed to open stream: Too many open files in /var/www/morningcroissant/symfony/vendor/symfony/error-handler/DebugClassLoader.php on line 349? Might not be the only ones having this issue

jtojnar · 2022-01-03T12:40:13Z

I would try installing the dependencies with --prefer-source and then bisecting individual dependencies – console and process are most likely suspects. (If you are unfamiliar, you can run git bisect start v5.3.13 v5.3.11 in the vendor/symfony/console directory, then run the tests and based on success/failure use git bisect good/bad in the dependency directory. After few steps, it should either find the commit that might have broken it, or you can use git bisect reset and try another dependency.)

adrienfr · 2022-01-03T18:08:07Z

I would try installing the dependencies with --prefer-source and then bisecting individual dependencies – console and process are most likely suspects. (If you are unfamiliar, you can run git bisect start v5.3.13 v5.3.11 in the vendor/symfony/console directory, then run the tests and based on success/failure use git bisect good/bad in the dependency directory. After few steps, it should either find the commit that might have broken it, or you can use git bisect reset and try another dependency.)

Thanks @jtojnar for your help! symfony/console & symfony/process are OK, the issue seems to come from symfony/http-client.
I try to find a specific commit but it might be related to this PR https://github.com/symfony/symfony/pull/44204/files, no more failed to open stream: Too many open files when reverting these 3 files to v5.3.12 :

cc @nicolas-grekas

adrienfr · 2022-01-03T18:47:55Z

I created an issue (#44900) so other users can find it and for better tracking.

nicolas-grekas

I'm 👎 here because this is not needed: PHP closes resources automatically when they are destructed. Also because right now this is a guess that would need a proof to turn into a merge.

#44900 looks like a better start to find the underlying issue. @jtojnar does you issue happen when HttpClient is used? Is it fixed when you revert to an older version of it?

jtojnar · 2022-01-04T19:46:20Z

I'm -1 here because this is not needed: PHP closes resources automatically when they are destructed.

I have not seen any much documentation an PHP resource management other than proc_open docs, which say that we should close the handles when we are done with them.

does you issue happen when HttpClient is used

As mentioned in the other thread, I do not have HttpClient in my use case.

I did some more digging and it appears that fopen('php://stdout') is responsible. Will try closing more of those.

jtojnar · 2022-01-04T20:08:29Z

I confirmed that the leak originates from this line:

symfony/src/Symfony/Component/Console/Output/ConsoleOutput.php

Line 152 in 4b41d17

return @fopen('php://stdout', 'w') ?: fopen('php://output', 'w');

We need to fclose this.

jtojnar · 2022-01-17T17:27:53Z

It seems that the test suite that triggers my issue caches configuration object for each new test. Since each configuration creates a new ConsoleOutput, which creates a new file descriptor, this causes the leak. But even explicitly closing the descriptor would not help, since the configuration objects are not destroyed before the test suite finishes.

On Symfony site, this could be prevented by caching the file descriptor of stdout (e.g. inside static property of ConsoleOutput) to avoid creating unnecessary file descriptors but I am not sure if this is desirable.

nicolas-grekas · 2022-01-17T17:30:27Z

Or by using the STDOUT const on the cli sapi I guess? Can you give it a try and see if that fixes your issue?

nicolas-grekas · 2022-01-18T09:15:52Z

Closing in favor of #45053

@jtojnar

…too many file descriptors (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Console] use STDOUT/ERR in ConsoleOutput to save opening too many file descriptors | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #44882 | License | MIT | Doc PR | - /cc @jtojnar can you please give this patch a try and report back? Commits ------- 6eaf9e6 [Console] use STDOUT/ERR in ConsoleOutput to save opening too many file descriptors

jtojnar requested review from chalasr and dunglas as code owners December 31, 2021 23:26

carsonbot added Status: Needs Review Bug labels Dec 31, 2021

carsonbot added this to the 6.1 milestone Dec 31, 2021

jtojnar changed the base branch from 6.1 to 5.3 December 31, 2021 23:27

jtojnar added 3 commits January 1, 2022 00:30

Console: close tty detection process

a91d25d

Console: close php://input handle

38c3583

Process: close process handles

454ce55

jtojnar force-pushed the close-resources branch from 4a29974 to 454ce55 Compare December 31, 2021 23:30

jtojnar mentioned this pull request Dec 31, 2021

Free cursor resources symfony/console#77

Closed

jtojnar mentioned this pull request Jan 1, 2022

Test suite exhausts file descriptors rectorphp/rector#6904

Closed

nicolas-grekas requested changes Jan 4, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 4, 2022

Nyholm added Console Process labels Jan 4, 2022

carsonbot changed the title ~~Free process resources~~ [Console][Process] Free process resources Jan 4, 2022

nicolas-grekas mentioned this pull request Jan 18, 2022

[Console] use STDOUT/ERR in ConsoleOutput to save opening too many file descriptors #45053

Merged

nicolas-grekas closed this Jan 18, 2022

jtojnar deleted the close-resources branch January 18, 2022 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console][Process] Free process resources #44882

[Console][Process] Free process resources #44882

jtojnar commented Dec 31, 2021

carsonbot commented Dec 31, 2021

nicolas-grekas commented Jan 1, 2022

jtojnar commented Jan 1, 2022

jtojnar commented Jan 1, 2022

adrienfr commented Jan 3, 2022

jtojnar commented Jan 3, 2022

adrienfr commented Jan 3, 2022 •

edited

Loading

jtojnar commented Jan 3, 2022 •

edited

Loading

adrienfr commented Jan 3, 2022 •

edited

Loading

adrienfr commented Jan 3, 2022

nicolas-grekas left a comment •

edited

Loading

jtojnar commented Jan 4, 2022

jtojnar commented Jan 4, 2022

jtojnar commented Jan 17, 2022

nicolas-grekas commented Jan 17, 2022 •

edited

Loading

nicolas-grekas commented Jan 18, 2022

[Console][Process] Free process resources #44882

[Console][Process] Free process resources #44882

Conversation

jtojnar commented Dec 31, 2021

carsonbot commented Dec 31, 2021

nicolas-grekas commented Jan 1, 2022

jtojnar commented Jan 1, 2022

jtojnar commented Jan 1, 2022

adrienfr commented Jan 3, 2022

jtojnar commented Jan 3, 2022

adrienfr commented Jan 3, 2022 • edited Loading

jtojnar commented Jan 3, 2022 • edited Loading

adrienfr commented Jan 3, 2022 • edited Loading

adrienfr commented Jan 3, 2022

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

jtojnar commented Jan 4, 2022

jtojnar commented Jan 4, 2022

jtojnar commented Jan 17, 2022

nicolas-grekas commented Jan 17, 2022 • edited Loading

nicolas-grekas commented Jan 18, 2022

adrienfr commented Jan 3, 2022 •

edited

Loading

jtojnar commented Jan 3, 2022 •

edited

Loading

adrienfr commented Jan 3, 2022 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas commented Jan 17, 2022 •

edited

Loading