WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#36621 closed defect (bug) (fixed)

Don't cache the results of wp_mkdir_p() in a persistent cache

Reported by: barry Owned by: ocean90
Milestone: 4.5.1 Priority: normal
Severity: major Version: 4.5
Component: Media Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Follow-up to #34359.

This broke things on WordPress.com and elsewhere. I don't think it's safe to cache the results of wp_mkdir_p() in a persistent cache, but caching it non-persistently seems fine. I think we need to go back to the approach in https://core.trac.wordpress.org/attachment/ticket/34359/34359.3.patch, storing the results in a non-persistent cache.

Attachments (2)

36621.patch (1.4 KB) - added by ocean90 4 years ago.
36621.1.patch (1.4 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (13)

@ocean90
4 years ago

#1 follow-up: @ocean90
4 years ago

  • Keywords has-patch added; needs-patch removed

@barry Out of interest, can you share what was broken exactly?

This ticket was mentioned in Slack in #core by azaozz. View the logs.


4 years ago

#3 in reply to: ↑ 1 @barry
4 years ago

Replying to ocean90:

@barry Out of interest, can you share what was broken exactly?

Essentially the exact thing mentioned in this comment. The WordPress.com use case is a bit more complicated, but basically we ended up caching a value for the upload dir from operations run on server A and when operations ran on server B and the directory didn't exist, the upload just fails because of the persistent cache, rather than trying to create the directory.

#4 @azaozz
4 years ago

36621.patch looks good here.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


4 years ago

@azaozz
4 years ago

#6 @azaozz
4 years ago

Looking more: there's no need to check if $tested_paths is array as it is set just before that. In 36621.1.patch.

#7 @swissspidy
4 years ago

  • Keywords commit added

#8 @ocean90
4 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 37285:

Media: Don't cache the results of wp_mkdir_p() in a persistent cache.

To improve the performance of wp_upload_dir() the result of wp_mkdir_p() was stored in a persistent cache, introduced in [36565]. But this becomes an issue when WordPress is scaled horizontally. You may end up caching a value for a server where the directory doesn't exist which will prevent further uploads on other servers because of the persistent cache.
The fix is to use a non-persistent cache.

Props azaozz, ocean90.
See #34359.
Fixes #36621.

#9 @ocean90
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#10 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37286:

Media: Don't cache the results of wp_mkdir_p() in a persistent cache.

To improve the performance of wp_upload_dir() the result of wp_mkdir_p() was stored in a persistent cache, introduced in [36565]. But this becomes an issue when WordPress is scaled horizontally. You may end up caching a value for a server where the directory doesn't exist which will prevent further uploads on other servers because of the persistent cache.
The fix is to use a non-persistent cache.

Merge of [37285] to the 4.5 branch.

Props azaozz, ocean90.
See #34359.
Fixes #36621.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.