Yoast / wordpress-seo

Yoast SEO for WordPress
https://yoast.com/wordpress/plugins/seo/
Other
1.77k stars 894 forks source link

Call `opcache_reset` in upgrade routine #9315

Closed moorscode closed 6 years ago

moorscode commented 6 years ago

As support is receiving reports that can be resolved by refreshing the PHP OPCache from users after every release, we want to try to trigger that refresh.

This means that we want to call the opcache_reset function, if it exists, whenever a new Yoast SEO version is detected.

This function is available since PHP 5.5, so wrapping it in a function_exists is required.

schlessera commented 6 years ago

To avoid unnecessarily impacting the performance of a site in a negative way, you could look into using opcache_invalidate( $script, true ) instead, so as to only invalidate the files that are part of the wordpress-seo plugin.

moorscode commented 6 years ago

We have considered that option. This would require us to create or store a list of all files that contain code in our plugin.

If we are going to do a dynamic determination, that would impact the performance about the same way as just doing a full opcache_reset (educated guess).

The question about performance impact has been asked at WordPress hosting companies. Waiting to get answers from them, meanwhile we want to be as safe as can be and just implement opcache_reset until other options have proven to make more sense.

pySilver commented 6 years ago

@schlessera We are observing huge performance impact due to this update. Somehow this reset is called on every call so, yeah – we are facing constant restarts of opcache. Somehow version check fails to work properly.

pySilver commented 6 years ago

Actually this is very harmful and unusual approach to solve things. Please consider removal of this this line.

moorscode commented 6 years ago

@pySilver I'm sorry you're having problems with the opcache_reset being called that many times. It should only do this when the constant WPSEO_VERSION differs from the one stored in the Database (or Object Cache, if applicable).

Could you create an issue with details about your setup (Object Caching or not) and the specific problem you are describing? This way we can track it and work towards a solution.

pySilver commented 6 years ago

@moorscode yeah I see the logic in code, my issue wasn't originally caused by Yoast itself (env made Yoast believed it hasn't updated, so it was running on each request causing high load sites to die). But thats not the point. Point is – opcache functions are very low level functions which usually related to deployment/devops/service scripts so I really cannot see a good reason why it's included into application level.

I've seen users complaining about things that was related to opcache (class not found and so on) – but thats not Yoast fault at all. Its their faulty environment issue. Opcache is shared mem based cache and its not a good idea if any app can clear it for all other apps for any reason.

moorscode commented 6 years ago

@pySilver I hear what you are saying. The reason why we have implemented this is because we have a steady amount of problems from users that update our plugin and have opcache not clearing the cached versions of files.

This results in fatal errors, which the users have a hard time resolving because they are not technical or don't have access to opcache.

The added call is an attempt to reduce these problems, with the knowledge that it might cause some extra clearing for files that are not related to our plugin.

The choice to not create a list of all the files (and the removed files between versions) has been made to reduce complexity, throwing a large net to catch some small fishes. As this is cache we are talking about, clearing it once when a plugin version changes should be handle-able by the server. We are monitoring the problems that this might be causing to determine if this solution is the most optimal. We realize that this is a hard thing to monitor, so it's mainly based on the number of reports -not- coming in in relation to other releases.

If you come across actual situations where this is causing big problems, please do not hesitate to let us know.

pySilver commented 6 years ago

@moorscode thanks for feedback. We may try to use invalidate scripts in loop. it's a little bit more complex, but can be easily achieved with glob() against plugin dir. But still it's more safe.

The problem I was facing was the following: Redis object cache failed to update plugin version (while db was updated) so it held outdated version making outside code to believe we need to upgrade and clear cache on every request. What was really funny is that while making update_option (which is executed after plugin upgrade) WP is not trying to update object cache because db enty did not change. So object cache was never updated.

moorscode commented 6 years ago

We may try to use invalidate scripts in loop. it's a little bit more complex, but can be easily achieved with glob() against plugin dir. But still it's more safe.

That will only cover the files that are currently there, but the files that have been removed in a release should be dropped from the cache as well. Minor thing about glob is also that it's not a very light or system-efficient call. And if caching of the filesystem is present during the glob, things get complicated as well.. which is why we opted for this implementation (for now).

The problem I was facing was the following: Redis object cache failed to update plugin version (while db was updated) so it held outdated version making outside code to believe we need to upgrade and clear cache on every request. What was really funny is that while making update_option (which is executed after plugin upgrade) WP is not trying to update object cache because db enty did not change. So object cache was never updated.

Argh! 🤕

lkraav commented 5 years ago

I now traced down our massive PHP performance problem down to the same thing @pySilver said in https://github.com/Yoast/wordpress-seo/issues/9315#issuecomment-388587925

In our WP instances also, opcache_reset() has been getting called on every single request. This logic at https://github.com/Yoast/wordpress-seo/blob/11.9-RC5/wp-seo-main.php#L301 seems to have issues and needs further attention.

We're also using Redis object cache for backing.

Is it safe to manually mod this option value? But is there a risk of relapse on further updates?

cc @moorscode

EDIT wp cache flush is the solution. WPSEO then runs its upgrade routine and manages to update the option value.

moorscode commented 5 years ago

Hi @lkraav,

Thanks for doing the digging and sharing it with us.

Could you provide a bit more info on where and how wp_cache_flush would be called and how it works in relation to this problem?

lkraav commented 5 years ago

Could you provide a bit more info on where and how wp_cache_flush would be called and how it works in relation to this problem?

I was referring to wp cache flush as in WP-CLI command.

rparrett commented 5 years ago

We have been affected by this "opcache_reset is called on every page load" issue recently, and it has likely cost us thousands of dollars.

I'm still not completely sure what's going on. I can't reproduce the issue locally.

My wild guess is that when combining:

The old version number is written over the new after the upgrade process by a request that began before the upgrade process.

I'd like to take this opportunity to add one more voice saying "please don't ever nuke my entire cache." which is something I unfortunately have been needing to ask pretty frequently of plugin authors lately. (Although I concede that this wouldn't be a huge deal to do only once when updating)

As a workaround, we are taking this strategy:

redis-cli --scan --pattern *options:alloptions | xargs redis-cli del might be a slightly less nuclear way of getting the cached options back in the right state (versus nuking the whole object cache when your server is already heavily loaded)

lkraav commented 5 years ago

Be super paranoid when updating wordpress-seo

Yep, this is now part of my toolset.

Basically, as-is, updating wordpress-seo requires a script, that still requires manual attendance:

Something to that effect.