Reduce scenarios where DB writes are performed during frontend requests

[westonruter]

There are a few places where DB writes are being performed during plugin initialization or when performing normal frontend requests to the site:

amp-wp/amp.php

Lines 404 to 420 in f1af0ab

	/*
	* Broadcast plugin updates.
	* Note that AMP_Options_Manager::get_option( 'version', '0.0' ) cannot be used because
	* version was new option added, and in that case default would never be used for a site
	* upgrading from a version prior to 1.0. So this is why get_option() is currently used.
	*/
	$options = get_option( AMP_Options_Manager::OPTION_NAME, array() );
	$old_version = isset( $options['version'] ) ? $options['version'] : '0.0';
	if ( AMP__VERSION !== $old_version ) {
	/**
	* Triggers when after amp_init when the plugin version has updated.
	*
	* @param string $old_version Old version.
	*/
	do_action( 'amp_plugin_update', $old_version );
	AMP_Options_Manager::update_option( 'version', AMP__VERSION );
	}

(Removed class-amp-story-templates.php instance since code removed in #4315.)

(Removed AMP_Theme_Support::check_for_cache_misses() example since removed in #4178.)

These can potentially cause race conditions where concurrent requests both cause an update to be done at the same time. It's also not great for performance to have DB writes being performed in response to frontend requests.

I suggest that each of these cases be refactored with wp_schedule_single_event() which can then be used to perform the writes during WP Cron, and thus avoid the race condition.

It's true that scheduling a WP Cron event is itself a DB write, but at least this a more proper way to do it. Otherwise, there could be a repeating task every X minutes to see if the above tasks need to be performed.

[archon810]

The primary reason we critically need something to be done here is in a master/slave WP env (using https://github.com/stuttter/ludicrousdb), after a master reboot, AMP plugin settings got completely reverted to their defaults.

I was able to replicate this twice after rebooting the master db server, and while it's unclear exactly why it happens or which race condition does it, it's very likely the cause is the code above that runs on every page load and initializes AMP plugin options if some data it expects isn't there (like AMP plugin version from db).

[westonruter]

@archon810 Please comment out those lines above in the plugin and monitor if you ever see the issue occur.

[archon810]

We're not planning on restarting the master for a while (it was a planned OS upgrade), and the changes would be wiped on the next plugin update when I run composer unless I clone the project into our local repo.

I wish I had a more reliable way to reproduce this :(

[westonruter]

Good point by @schlessera in that some of these should continue to be synchronous, but gated behind requests to the admin.

[archon810]

Absolutely agreed.

[archon810]

What's the status of this issue please? I haven't observed the bug lately but we haven't been restarting the master much, and it's not a guarantee that the bug is fixed.

[westonruter]

There is no status update. It hasn't been prioritized since we haven't had any other reports of this being an issue, and you haven't been able to reliably reproduce it.

A related issue is #3287 where amp-options could be made a non-autoloaded option, which may be related to the error you encountered. That being said, an update in WordPress 5.3.1 may actually fix this. See https://core.trac.wordpress.org/ticket/31245.

[westonruter]

A related issue is #3287 where amp-options could be made a non-autoloaded option, which may be related to the error you encountered. That being said, an update in WordPress 5.3.1 may actually fix this. See https://core.trac.wordpress.org/ticket/31245.

I was mistaken. The amp-options option is actually not autoloaded at present, so this is not relevant.

[westonruter]

Note the instances of frontend writes have been reduced in 1.5 since:

• class-amp-story-templates.php was removed in Remove Stories codebase #4315.
• AMP_Theme_Support::check_for_cache_misses() was removed in Eliminate post-processor cache #4178.

The only remaining case is in amp_init() to trigger the plugin update routine.

One idea for how to handle that instead is to put it inside of an admin_init hook as explained in https://developer.wordpress.org/reference/functions/register_activation_hook/#comment-2100

[westonruter]

The DB upgrade routines don't happen on the frontend either, as you recall the upgrade DB message is only presented once trying to access the admin.

[archon810]

As I mentioned to @westonruter, the plugin settings reset happened again for us, running 1.4.4. Looking forward to the new fixes in 1.5 and the one in the PR above.

[westonruter]

@archon810 Nevertheless, if you didn't upgrade then the update routine in #4538 would not have ran. Therefore, the change in #4391 may actually be what fixes the issue for you, if frontend DB writes are indeed the problem.