WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#16389 closed defect (bug) (fixed)

register_shutdown_function isn't run in SHORTINIT mode

Reported by: jtclarke Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: major Version: 3.0.4
Component: Multisite Keywords: has-patch 3.3-early
Focuses: Cc:

Description

I was noticing problems upgrading from 2.9.2 MU to 3.0.4 where some of my images weren't loading correctly through ms-files.php when I enabled the drop-in object-cache.php (in my case Ryan et al's memcache client).

I tracked the problem down to an inconsistency between MU and the new 3.0 codebase in the wp-settings.php file.

The problem is that if the SHORTINIT constant is set (as it is in ms-files.php), it returns false (at line 94) before we the shutdown function is registered (line 271). The shutdown function primarily takes care of object cash closing -- which needs to take place in both the short and regular load sequences.

donncha fixed this in the MU codebase at revision 1582 in the wordpress-mu SVN tree (between 2.6 and 2.7), but it looks like this change didn't get carried over in the 3.0 merge.

I recommend we restore the fix from 1582 (partially) by moving register_shutdown_function above the SHORTINIT check.

This should only affect multisite installations (the only ones that use SHORTINIT), but should fix the problem

Attachments (5)

wp-settings.diff (653 bytes) - added by jtclarke 3 years ago.
wp-settings.php patch for SHORTINIT
screenshot1.png (162.2 KB) - added by jtclarke 3 years ago.
Batcache enabled, wp-admin file upload
screenshot2.png (185.7 KB) - added by jtclarke 3 years ago.
Batcache disabled, wp-admin file upload
screenshot3.png (166.1 KB) - added by jtclarke 3 years ago.
Batcache re-enabled, wp-admin file upload
screenshot4.png (185.9 KB) - added by jtclarke 3 years ago.
Batcache enabled, fix implemented -- wp-admin file upload

Download all attachments as: .zip

Change History (27)

comment:1 jtclarke3 years ago

  • Keywords SHORTINIT register_shutdown_function multisite removed

jtclarke3 years ago

wp-settings.php patch for SHORTINIT

comment:2 jtclarke3 years ago

  • Keywords multisite has-patch dev-feedback added

the attached diff file resolves the issue with SHORTINIT.

please confirm.

comment:3 jtclarke3 years ago

  • Cc jtclarke added

comment:4 jtclarke3 years ago

  • Component changed from General to Multisite

comment:6 westi3 years ago

  • Cc westi added
  • Keywords has-path reporter-feedback added; multisite has-patch dev-feedback removed

What part of ms-files.php is broken by this code being where it is now?

In other words how can I reproduce the issue that this change fixes - there is little history to say why this should be done :)

comment:7 SergeyBiryukov3 years ago

  • Keywords has-patch added; has-path removed

comment:8 follow-up: nacin3 years ago

  • Keywords reporter-feedback removed

Sounds like this is for an object cache needing to leverage the wp_cache_close() call in the shutdown handler. Given the OP stating the Memcached plugin in use, and that the MU changeset gives props to Ryan, and that the Memcached plugin uses wp_cache_close()... I'm removing reporter-feedback. :)

Given this was something missing in the merge, and moving this has basically no ill side effects, this looks good.

comment:9 in reply to: ↑ 8 westi3 years ago

  • Keywords reporter-feedback added

Replying to nacin:

Sounds like this is for an object cache needing to leverage the wp_cache_close() call in the shutdown handler. Given the OP stating the Memcached plugin in use, and that the MU changeset gives props to Ryan, and that the Memcached plugin uses wp_cache_close()... I'm removing reporter-feedback. :)

Given this was something missing in the merge, and moving this has basically no ill side effects, this looks good.

Yes but from a quick glance I don't see anything causing cache writes in ms-files.php

This has been gone for a long time so while it is probably prudent to move it to the place it was in mu - we should know why!

There was no explanation as to what needed this to move.

Hence the request for more info from the reporter.

comment:10 jtclarke3 years ago

Hey -- sorry I somehow wasn't getting requests for feedback.

The way I saw this was that on the sites I manage (MU with Batcache / Memcache), some (but not all) of the images were not being served correctly when Batcache was enabled and memcached was turned on.

I too am surprised nobody has encountered this before. Technically, the only way you would see problems is using 3.0 multisite (which uses ms-files.php and SHORTINIT) with Batcache/Memcache -- but I can't imagine we're the only ones doing that.

It's unclear why it would affect some files and not all. But I know that the problem was fixed if I turned off SHORTINIT -- which led me to dig deeper into the MU code and the placement of register_shutdown_function.

Let me provide some concrete steps to reproduce. Hopefully that'll clear things up. But it certainly was done in the MU codebase for a reason -- and the fact that it wasn't carried over to 3.0 was an oversight, not a choice.

Last edited 3 years ago by jtclarke (previous) (diff)

comment:11 jtclarke3 years ago

Steps to reproduce:

Using a multisite installation of Wordpress 3.0.4, the TwentyTen theme and no plugins enabled (to eliminate any extraneous variables) and ensuring my system has the PHP5-Memcache extension:

  1. Drop in Ryan et al's memcache client (object-cache.php.)
  2. Start memcached
  3. Begin creating new posts in wp-admin. With each new post, upload an image via the media uploader and try inserting as many sizes as are available into the post / Featured Image.

For me, even on the first post, what I see is that only some of the image sizes are served correctly. For example. I see the Full size and Post Thumb, but not the 150x150 thumbnail. See screenshot1.png. This affects the thumb in both wp-admin and on the frontend.

If I try loading the failing image directly from its url (eg. http://flavordev.local.mu/files/2011/02/6413719-150x150.jpg), the image does not load -- all I get is the url string.

When I remove object-cache.php, all images appear as they should. See screenshot2.png.

If I restore object-cache.php (making sure to clear my browser cache) the 150x150 disappears again. See screenshot3.png.

If I leave object-cache.php in place and institute the change I suggested in the diff file, everything serves as it should. See screenshot4.png.

Let me know if you need any more information.

Last edited 3 years ago by jtclarke (previous) (diff)

jtclarke3 years ago

Batcache enabled, wp-admin file upload

jtclarke3 years ago

Batcache disabled, wp-admin file upload

jtclarke3 years ago

Batcache re-enabled, wp-admin file upload

jtclarke3 years ago

Batcache enabled, fix implemented -- wp-admin file upload

comment:12 jtclarke3 years ago

  • Keywords dev-feedback added; reporter-feedback removed

comment:13 jtclarke3 years ago

As a side note, I'm not the first person to see this issue. See: http://wordpress.org/support/topic/wordpress-30-multisite-mode-and-object-cache-with-memcached-not-showing-images?replies=4

It seems like Fernando solved the problem by installing a newer version of the PECL Memcache client. But the client he recommends is still in beta -- which isn't ideal.

The client I use on my environment is the latest stable (2.2.6) and with the mod suggested in the diff file, everything works as it should.

comment:14 jtclarke3 years ago

hello? anyone?

comment:15 jtclarke3 years ago

  • Severity changed from normal to major

comment:16 jtclarke3 years ago

  • Version changed from 3.0.4 to 3.1

This is still a problem in 3.1. Modifying ticket accordingly.

comment:17 coffee2code3 years ago

  • Version changed from 3.1 to 3.0.4

The version field is used to report when the issue was first observed/reported. A comment to indicate that it still applies in 3.1 (as you've done) and the fact that the ticket remains opened is sufficient to keep the ticket up-to-date.

comment:18 jtclarke3 years ago

  • Keywords 2nd-opinion added

Could someone give this bug some love? It's done. It's fixed. it's been sitting here for 5 months.

comment:19 ryan3 years ago

  • Keywords 3.3-early added; dev-feedback 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

Sorry. This slipped off the radar. The patch seems good, and I'm marking this for early inclusion into 3.3 since we missed the 3.2 boat.

comment:20 jtclarke3 years ago

sweet. thanks Ryan :)

comment:21 ryan3 years ago

  • Milestone changed from Future Release to 3.3

comment:22 ryan3 years ago

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

In [18450]:

Call register_shutdown_function() for SHORTINIT. Props jtclarke. fixes #16389

Note: See TracTickets for help on using tickets.