WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 6 months ago

#24958 new defect (bug)

Large number of revisions cause memory exhaustion

Reported by: jshreve Owned by:
Milestone: Future Release Priority: low
Severity: normal Version: 3.6
Component: Revisions Keywords: has-patch
Focuses: Cc:

Description

This may be a edge case, but if you have a a large number of revisions, a number of things can break.

Right now the post in question has about 1,055 revisions.

Noticeably, two things are happening:

Calling wp_update_post from a plugin fails with the following error:

PHP Fatal error: Allowed memory size of 41943040 bytes exhausted (tried to allocate 16384 bytes) in /path/wp-includes/wp-db.php on line 1228

Using the new revisions API to view earlier revisions returns

"Sorry, something went wrong. The requested comparison could not be loaded."
and the error log contains the same memory exhaustion error.

I can seem to update from the post edit screen.

This is happening with the latest stable of 3.6 and only began happening after an update, so I suspect it's something in the new revision core/api. I haven't had a huge amount of time to investigate but my guess is it's trying to load too much revision data at one time? Maybe it should only try to load X amount of revisions at once?

Attachments (1)

24958.diff (602 bytes) - added by nacin 8 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 in reply to: ↑ description jshreve8 months ago

Replying to jshreve:

Using the new revisions API to view earlier revisions returns

I meant to say "new revisions UI" here.

comment:2 follow-up: adamsilverstein8 months ago

Wow, 1000+ revisions - that does seem like an edge case... none the less, crashing PHP is bad form and I agree we need some checks in place to make sure we don't do that. I'm not sure about the first issue - the second one may be caused because the revisions page tries to pre-load all of the initial revision comparisons (so you can rapidly scroll thru them). The code is already capable of lazy-loading the data, and in fact thats what happens in the 'compare any two revisions' mode, we should be able to leverage that for large data sets like yours.

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6 we tried posts with several hundred revisions, but never got to 1000!

comment:3 adamsilverstein8 months ago

  • Cc adamsilverstein@… added

comment:4 nacin8 months ago

  • Milestone changed from Awaiting Review to 3.6.1

wp_update_post() triggers save_post which calls wp_save_post_revision() which calls wp_get_post_revisions() to get the last revision.

We should set up wp_get_post_revisions() to be able to request the last non-autosave in a single query, rather than potentially loading all revisions into memory. I almost did this in 3.6 for wp_get_post_autosave() because it used to do a direct WP_Query for an autosave, and now it calls wp_get_post_revisions(), which can be painful.

This should fix the issue of wp_update_post(). It won't fix the revisions screen, which *does* have some failover if it takes too long (or too much memory) to process all of the diffs, but it does still try to load all revisions at once. We should place a limit on that actual query, say 200, to force it to occur in chunks. Our JS will handle it automatically.

comment:5 in reply to: ↑ 2 ; follow-up: jshreve8 months ago

Replying to adamsilverstein:

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6
we tried posts with several hundred revisions, but never got to 1000!

The reason this was surfaced is because we have a test suite for the WordPress.com REST API that edits the same post over and over - therefor it quickly got to 1000+ revisions :). We could/should probably clear out the data after the tests run for certain things - but that's how it got so high.

comment:6 in reply to: ↑ 5 adamsilverstein8 months ago

Replying to jshreve:

Replying to adamsilverstein:

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6
we tried posts with several hundred revisions, but never got to 1000!

The reason this was surfaced is because we have a test suite for the WordPress.com REST API that edits the same post over and over - therefor it quickly got to 1000+ revisions :). We could/should probably clear out the data after the tests run for certain things - but that's how it got so high.

Tearing down your old test data would make sense, also you can limit the number of revisions WordPress stores, see wp_revisions_to_keep. In any case, we should still address the issues you raise in this ticket.

comment:7 SergeyBiryukov8 months ago

  • Version set to 3.6

comment:8 follow-up: nacin8 months ago

My comment was pretty in depth, which shouldn't make it too difficult for someone to come up with a patch, but it is still fairly complicated, and might require some enhancements to WP_Query or a less-than-triumphant return of _wp_get_post_autosave_hack() to do some hijinks. Or, a direct query.

For 3.6.1 I think we should either do @ini_set( 'memory_limit', apply_filters( 'admin_memory_limit', WP_MAX_MEMORY_LIMIT ) ); or do 'posts_per_page' => 1000.

That said, WordPress.com is running an opcode cache (i.e. APC) which means the actual memory of the current process is possibly lower than what might be typical in the non-opcode admin (mid 20s MB). (This might not be true since WP.com runs a metric ton of extra code.) We should test how much memory 1000 revisions actually requires (and compare to both frontend and admin) and see if limiting to 1000 makes sense.

I'm fine with limiting to 1000 (if that's not too high) as a "dude, seriously?" number to avoid fatal errors. And it is, of course, filterable.

comment:9 in reply to: ↑ 8 SergeyBiryukov8 months ago

Replying to nacin:

We should test how much memory 1000 revisions actually requires (and compare to both frontend and admin) and see if limiting to 1000 makes sense.

  1. Created 1000 revisions (in trunk, PHP 5.2.14, Windows) using ocean90's script.
  2. On the Revisions screen, Debug Bar shows "Memory Usage: 34 346 816 bytes".
  3. The same install shows 18 804 216 bytes on the front-end, and 25 544 016 bytes on Dashboard screen.

nacin8 months ago

comment:10 nacin8 months ago

  • Keywords has-patch commit added

Thanks, Sergey! OK, so this is something we might only hit if we also have a ton of plugins and creep close to the 40 MB limit. I am actually very surprised WP.com isn't either far above that, or deliberately -1.

24958.diff implements a soft 1000 limit. I say "soft" because you can change it to -1 via $args for wp_get_post_revisions(). It is not enforced and thus does not need to be changed by reaching into a WP_Query filter. I think this is good for 3.6.1.

comment:11 nacin7 months ago

  • Keywords commit removed

1000 is lame for when order => ASC, which we do in a few cases; and I don't actually see a change in handling that from 3.5.

comment:12 nacin7 months ago

  • Milestone changed from 3.6.1 to 3.6.2
  • Priority changed from normal to low

comment:13 johnbillion6 months ago

  • Milestone changed from 3.6.2 to Future Release

After discussion at WCEU we're punting this due to edge case scenario and no immediate fix available.

Note: See TracTickets for help on using tickets.