Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Mime] Close file handle #58163

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
Closed
wants to merge 2 commits into from
Closed

[Mime] Close file handle #58163

wants to merge 2 commits into from

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Sep 3, 2024 •
edited
Loading

Q A
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Currently the finfo file handle does not get closed which can result in the error
Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php
if guessMimeType() gets called for more than ulimit -n files.

According to https://stackoverflow.com/a/32102146 the file handle can be closed with unset() - as it is done in PHP's unit tests

@carsonbot carsonbot added this to the 7.1 milestone Sep 3, 2024
@BlackbitDevs BlackbitDevs changed the title close file handle [FileinfoMimeTypeGuesser| Close file handle Sep 3, 2024
@BlackbitDevs BlackbitDevs changed the title [FileinfoMimeTypeGuesser| Close file handle [FileinfoMimeTypeGuesser] Close file handle Sep 3, 2024
@BlackbitDevs BlackbitDevs marked this pull request as draft September 3, 2024 14:50
@BlackbitDevs BlackbitDevs marked this pull request as ready for review September 3, 2024 14:56
@OskarStark OskarStark added the Mime label Sep 3, 2024
@carsonbot carsonbot changed the title [FileinfoMimeTypeGuesser] Close file handle [Mime] [FileinfoMimeTypeGuesser] Close file handle Sep 3, 2024
@OskarStark OskarStark changed the title [Mime] [FileinfoMimeTypeGuesser] Close file handle [Mime] Close file handle Sep 3, 2024
@xabbuh
Copy link
Member

xabbuh commented Sep 4, 2024

Did you actually run into the issue you describe? I would expect PHP to implicitly do the same already due to the fact that the method ends a few lines later.

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Sep 4, 2024 •
edited
Loading

Yes, I have an import script for Pimcore (which is based on Symfony) which imports files from an FTP resource (source is an ERP system). For every file being imported, the MIME type gets determined. The script runs fine during the month when there are only a few changes in ERP and thus only a few files to be imported. But at the beginning of a new month, a lot of prices or other things get changed and thus a lot of files have to be processed in one process. And in those runs my error log looks like this:

[WARNING] E_WARNING: finfo::file([...]/var/tmp/temp-file-66d5373181f41-5c3c6109d27865c619b1658d25eac4.tmp): failed to open stream: Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php, line 71
[WARNING] E_WARNING: fopen([...]/export/archive/2024/09/02/05-55-29-0803341_ItemMasterPIM.xml): failed to open stream: Too many open files in [...]/vendor/pimcore/pimcore/models/Asset.php, line 709
[WARNING] E_WARNING: finfo::file([...]/var/tmp/temp-file-66d5373181f41-5c3c6109d27865c619b1658d25eac4.tmp): failed to open stream: Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php, line 71
[WARNING] E_WARNING: fopen([...]/export/archive/2024/09/02/05-55-29-803341_ItemMasterPIM.xml): failed to open stream: Too many open files in [...]/vendor/pimcore/pimcore/models/Asset.php, line 709
[WARNING] E_WARNING: finfo::file([...]/var/tmp/temp-file-66d5373181f41-5c3c6109d27865c619b1658d25eac4.tmp): failed to open stream: Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php, line 71
[WARNING] E_WARNING: fopen([...]/export/archive/2024/09/02/05-55-29-03341_ItemMasterPIM.xml): failed to open stream: Too many open files in [...]/vendor/pimcore/pimcore/models/Asset.php, line 709
[WARNING] E_WARNING: finfo::file([...]/var/tmp/temp-file-66d5373181f41-5c3c6109d27865c619b1658d25eac4.tmp): failed to open stream: Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php, line 71
[WARNING] E_WARNING: fopen([...]/export/archive/2024/09/02/05-55-29-3341_ItemMasterPIM.xml): failed to open stream: Too many open files in [...]/vendor/pimcore/pimcore/models/Asset.php, line 709
[WARNING] E_WARNING: finfo::file([...]/var/tmp/temp-file-66d5373181f41-5c3c6109d27865c619b1658d25eac4.tmp): failed to open stream: Too many open files in [...]/vendor/symfony/symfony/src/Symfony/Component/Mime/FileinfoMimeTypeGuesser.php, line 71

Of course only one of those messages is from Symfony, the other one is from Pimcore but Pimcore closes the stream after saving the file, see https://github.com/pimcore/pimcore/blob/32f3f344a59ec3ddf8570c58443b9d2a86f25758/models/Asset.php#L798

But when I look at the PHP source code, maybe you are right because in https://github.com/php/php-src/blob/fad899e5662d0a929d8462dd8239b0489dd9b53f/ext/fileinfo/fileinfo.c#L400C14-L400C40 the stream gets opened and in https://github.com/php/php-src/blob/fad899e5662d0a929d8462dd8239b0489dd9b53f/ext/fileinfo/fileinfo.c#L415 it gets closed.

But then I only wonder why in the PHP tests unset() gets called in https://github.com/php/php-src/blob/8a2015451ac18b656e3fcd608bce9eb2f86db452/ext/fileinfo/tests/finfo_close_basic.phpt#L17-L19 because this should be the OOP equivalent of https://github.com/php/php-src/blob/8a2015451ac18b656e3fcd608bce9eb2f86db452/ext/fileinfo/tests/finfo_close_basic.phpt#L11-L15

@xabbuh
Copy link
Member

xabbuh commented Sep 4, 2024

Were you able to run the patch in the affected environment? Did it actually make a difference?

@nicolas-grekas nicolas-grekas mentioned this pull request Sep 11, 2024
Merged
@nicolas-grekas
Copy link
Member

I'm wondering the same as @xabbuh. The end of the method will free the variable anyway so this should make no difference.
This gave me the idea to cache finfo handles though, see #58233

nicolas-grekas added a commit that referenced this pull request Sep 13, 2024
@nicolas-grekas
minor #58233 [Mime] Cache finfo objects to reduce open file handles a…
4e70834 
…nd optimize perf (nicolas-grekas)

This PR was merged into the 7.2 branch.

Discussion
----------

[Mime] Cache finfo objects to reduce open file handles and optimize perf

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Inspired by #58163

Commits
-------

5a1be29 [Mime] Cache finfo objects to reduce open file handles and optimize perf
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
Bug Mime Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
5 participants
@BlackbitDevs @xabbuh @nicolas-grekas @OskarStark @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.