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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gallery: Upload images one at a time #11565

Merged
merged 4 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@noisysocks
Member

noisysocks commented Nov 7, 2018

Fixes #8935.

The problem 馃

Note: I've tested this all using VVV, but other development and production environments would be similar.

When 5-10 large (10 MB) images are uploaded at once using the Gallery block, it's highly likely that a few The response was not a valid JSON response. errors appear on the screen.

This is because the server has responded to some of the upload requests with a 502 Bad Gateway error. The body of these responses is a nginx HTML error page, which explains the 'Invalid JSON' message.

Tailing the nginx error log (/vagrant/www/wordpress-default/log/error.log) reveals that the error occurs because its connection to php-fpm was reset:

2018/11/07 06:35:31 [error] 5676#5676: *133 recv() failed (104: Connection reset by peer) while reading response header from upstream, client: 192.168.50.1, server: local.wordpress.test, request: "POST /wp-json/wp/v2/media?_locale=user HTTP/1.1", upstream: "fastcgi://unix:/var/run/php7.2-fpm.sock:", host: "local.wordpress.test", referrer: "http://local.wordpress.test/wp-admin/post-new.php"

Tailing the system log (/var/syslog) reveals that this is because the operating system ran out of memory and so decided to kill php-fpm:

Nov  7 05:54:49 vvv kernel: [62514.273979] Killed process 7868 (php-fpm7.2) total-vm:1613272kB, anon-rss:763208kB, file-rss:8212kB

You can see from the system log that the php-fpm workers were altogether consuming about 1.7 GB of memory鈥攅asily enough to cause an OOM in this virtual machine that's configured with 2 GB of RAM:

Nov  7 05:54:49 vvv kernel: [62514.273061] [ pid ]   uid  tgid total_vm      rss nr_ptes swapents oom_score_adj name
Nov  7 05:54:49 vvv kernel: [62514.273162] [ 7825]     0  7825   112500     2370     155        0             0 php-fpm7.2
Nov  7 05:54:49 vvv kernel: [62514.273164] [ 7868]    33  7868   403318   192855     692        0             0 php-fpm7.2
Nov  7 05:54:49 vvv kernel: [62514.273167] [ 7871]    33  7871   294628    90474     491        0             0 php-fpm7.2
Nov  7 05:54:49 vvv kernel: [62514.273169] [ 7884]    33  7884   399494    64932     667        0             0 php-fpm7.2
Nov  7 05:54:49 vvv kernel: [62514.273171] [ 7928]    33  7928   402001    90221     704        0             0 php-fpm7.2
Nov  7 05:54:49 vvv kernel: [62514.273173] [ 7930]    33  7930   244805    16504     343        0             0 php-fpm7.2

This is because WordPress generates thumbnails for each image that is uploaded. Generating thumbnails for a large image is very memory intensive. In our case, we're generating thumbnails for several large images at once.

I'm not entirely clear on why the PHP runtime itself doesn't throw an OOM exception from exceeding the memory_limit set in php.ini. My guess is that it's because image resizing happens in a seperate module running under the php-fpm process.

Proposed fix 馃槷

Both the Upload New Media page and the Media Library both handle uploading multiple images by uploading one image at a time. That is, we wait until the first POST request to async-upload.php finishes before starting on the second.

Borrowing this approach for Gutenberg is a nice fix for the above issue because:

  • It reduces load on the server as thumbnails are generated for only one image at a time.
  • It works across all of the different WordPress server environments we're likely to encounter.
  • It makes the Gallery block in Gutenberg consistent with the rest of WordPress.

It's also really simple to implement thanks to async and await.

How to test 馃搵

  1. Ensure that your PHP max upload size setting is high enough
  2. Create a new post
  3. Insert a Gallery block
  4. Click Upload
  5. Select ~10 large (10 MB) images
  6. All images should eventually upload without error.
  7. Check that uploading in other blocks has not regressed e.g. Image, File, Audio, Video

Future work 馃搯

There are some unrelated issues that should be fixed separately:

  • There is no indication that an image in the Gallery block is uploading.
  • There is a flash of white when an image in the Gallery block has uploaded because we swap out the blob: URL for a non-cached http:// URL.
Gallery: Upload images one at a time
Makes mediaUpload() upload image files one at a time. This matches the
existing Media Library's behaviour.

By waiting for each image to process before starting on the next, the
PHP/CGI backend isn't overloaded with requests to resize multiple image
files at the same time--something that always causes timeout
errors for large images.

@noisysocks noisysocks added this to the 4.3 milestone Nov 7, 2018

@noisysocks noisysocks requested review from talldan and WordPress/gutenberg-core Nov 7, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 7, 2018

Props to @talldan for pairing on this with me! 鉂わ笍

Ensure files that fail validation are skipped and trigger an error, b鈥
鈥t in the case of mulitple files, valid uploads continue to be processed

@talldan talldan removed the request for review from WordPress/gutenberg-core Nov 7, 2018

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 7, 2018

Removed the request for review since I think I've spotted an issue. Will pick up with @noisysocks tomorrow.

noisysocks added some commits Nov 8, 2018

Media Upload: Improve tests
Properly mock the modules that mediaUpload() uses so that we can more
easily test calling mediaUpload().
@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 8, 2018

I pushed up some changes which:

  • Fix a bug where we weren't creating all of the image blobs before starting to process image uploads.
  • Rewrites the mediaUpload() tests to properly mock @wordpress/api-fetch and @wordpress/blob instead of just asserting that an error was thrown.

We should look at writing more tests for mediaUpload(). Now that its external modules are mocked properly there should be nothing stopping us from writing tests that check that mediaUpload() calls apiFetch() properly.

We should also look at splitting mediaUpload() up into some smaller functions. It is fairly large and performs a lot of array mutation which makes it really difficult to understand.

But all these problems already exist in master and I'm conscious of spending too much time here on things unrelated to the original issue 馃檪

@ocean90

This comment has been minimized.

Member

ocean90 commented Nov 8, 2018

Kind of a bummer that we have to revert this. Look at this 11-year-old ticket from @m. 馃檪

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 8, 2018

Is this ready to be reviewed or still in progress?

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 8, 2018

@youknowriad Ready to review now. Just giving it a test.

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 8, 2018

Spotted a bug鈥攚hen an image fails validation it's replaced by the next one that passes. e.g. if you try to upload images:
A B C D

and C fails validation you actually end up with the following images in the gallery:
A B D D

I'll debug it now.

@youknowriad

LGTM 馃憤

This has been a long-standing issue, I'm happy to see it fixed.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 8, 2018

(I missed the bug, let me know if you need testing after that)

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 8, 2018

@youknowriad - My local branch wasn't up to date 馃う鈥嶁檪锔 - looks good, will merge.

@talldan talldan merged commit 69427ea into master Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the fix/upload-images-one-at-a-time branch Nov 8, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 8, 2018

Thanks @talldan and @youknowriad!

Kind of a bummer that we have to revert this. Look at this 11-year-old ticket from @m. 馃檪

Yeah, wow, in five years that ticket will be able to drive.

I'm not sure that we are able to solve the general problem here which is that the server can be overloaded from requests to resize images. Server admins would have to configure nginx and php-fpm so that there's a sufficient queue for requests. WordPress doesn't have a whole lot of say in what environment it runs on.

I'd say that going forward we could look at making it so that we upload two or three images at a time. But, for now, this fixes the issue and isn't any worse than what's already in Core.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 9, 2018

Wanted to create some issues for the future work identified here.

There is no indication that an image in the Gallery block is uploading.

This already sort of exists: #8810

There is a flash of white when an image in the Gallery block has uploaded because we swap out the blob: URL for a non-cached http:// URL.

Created: #11650

@aduth aduth referenced this pull request Nov 14, 2018

Closed

Fix "undo trap" by introducing "transient" attributes #11504

0 of 4 tasks complete

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Gallery: Upload images one at a time (WordPress#11565)
* Gallery: Upload images one at a time

Makes mediaUpload() upload image files one at a time. This matches the
existing Media Library's behaviour.

By waiting for each image to process before starting on the next, the
PHP/CGI backend isn't overloaded with requests to resize multiple image
files at the same time--something that always causes timeout
errors for large images.

* Ensure files that fail validation are skipped and trigger an error, but in the case of mulitple files, valid uploads continue to be processed

* Media Upload: Process blobs before uploading each image

* Media Upload: Improve tests

Properly mock the modules that mediaUpload() uses so that we can more
easily test calling mediaUpload().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment