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.

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

fix(files): memory usage in filter_wp_read_image_metadata() #5554

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

sjinks
Copy link
Member

@sjinks sjinks commented May 10, 2024

Description

Reduce memory usage in VIP_Filesystem::filter_wp_read_image_metadata() by using copy() instead of file_get_contents()/file_put_contents().

The symptoms are:

PHP message: PHP Fatal error:  Allowed memory size of 805306368 bytes exhausted (tried to allocate 5046659072 bytes) in /wp/wp-content/mu-plugins/files/class-vip-filesystem.php on line 454

Changelog Description

Fixed

  • Reduced memory consumption in the VIP File Service plugin when retrieving metadata of a large file.

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change works and has been tested on a sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

Upload a large file (its size should exceed the memory available to PHP) before and after this patch.

@sjinks sjinks self-assigned this May 10, 2024
@sjinks sjinks requested a review from a team as a code owner May 10, 2024 00:37
Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 29.43%. Comparing base (98fd1d3) to head (7a239f3).

Files Patch % Lines
files/class-vip-filesystem.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #5554      +/-   ##
=============================================
+ Coverage      29.41%   29.43%   +0.01%     
  Complexity      4764     4764              
=============================================
  Files            281      281              
  Lines          20544    20532      -12     
=============================================
  Hits            6043     6043              
+ Misses         14501    14489      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented May 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Adirael
Copy link

Adirael commented May 10, 2024

Folded in my branch with dynamic timeouts on the download path, it's an extra call to VIP FS and probably needs some more defensive code and testing :)

rinatkhaziev
rinatkhaziev previously approved these changes May 13, 2024
Copy link

sonarcloud bot commented Jun 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants