Sync: Add Async Sender #7482

lezama · 2017-07-18T17:56:23Z

Add the option to load the Jetpack Sync Sender on another request as core does it for cron*. (hat tip @westi).

This feature is Off by default, and can be turned on via a jetpack_sync_settings_async_sender filter

*We are avoiding just using spawn_cron as we already tried using it in the past, causing troubles with misconfigured cron setups.

How to test

turn on the feature(off by default):

Use the sync settings endpoint to set async_sender to 1
or
Using a filter: add_filter('jetpack_sync_settings_async_sender', '__return_true');
on the ngrok console you should see a separate request to admin-ajax.php just for syncing
alternatively follow @enejb's instructions on Sync: add fastcgi finish request to sync #11146 and check that the original request doesn't get delayed

sync/class.jetpack-sync-actions.php

enejb · 2018-05-02T19:16:10Z

@lezama Do we still want to take this approach?

lezama · 2018-05-04T07:22:56Z

what do you think about offering it as an alternative?

enejb · 2018-05-04T18:28:39Z

Sure should we put in as an option or a constant?
maybe something we can change via the sync settings on a site

lezama · 2018-05-07T07:59:55Z

maybe something we can change via the sync settings on a site

👍

stale · 2018-09-24T16:00:05Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

jetpackbot · 2019-01-14T18:36:06Z

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against bb6c2b7

enejb · 2019-01-14T23:42:09Z

This works as expected for me.

To test this. I did the following.

diff --git a/sync/class.jetpack-sync-sender.php b/sync/class.jetpack-sync-sender.php
index 4a5b678e3..7f9a42f38 100644
--- a/sync/class.jetpack-sync-sender.php
+++ b/sync/class.jetpack-sync-sender.php
@@ -103,6 +103,8 @@ class Jetpack_Sync_Sender {
        }

        public function do_sync() {
+               l( 'do SYNC ', getmypid() );
+               sleep( 10 );
                return $this->do_sync_and_set_delays( $this->sync_queue );
        }

Did an api call to the site.
Noticed that the api call returned in 10 seconds.

And then enabled the async_sender option by setting it to true.
via the sync manager on .com

And notice that the error log still showed an do SYNC call.
but the api response was much faster.

Based on the commit to core https://core.trac.wordpress.org/changeset/44488 That improves cron response. This PR tries to do the same but for Jetpack. This works together with #7482 To improve the page load response time when sync is progress.

sync/class.jetpack-sync-actions.php

enejb · 2019-01-22T23:53:57Z

@lezama can you review this again.

lezama · 2019-01-23T19:02:53Z

sync/class.jetpack-sync-sender.php

+		if ( $this->is_locked() ) {
+			$self = $this->get_instance();
+			// Remove any sync sending...
+			remove_action( 'shutdown', array( $self ,'do_sync' ) );


can't we use $this?

I am not sure maybe.

Based on the commit to core https://core.trac.wordpress.org/changeset/44488 That improves cron response. This PR tries to do the same but for Jetpack. This works together with #7482 To improve the page load response time when sync is progress. #### Testing instructions: * Go to '..' On an environment that support the function add the following to your code. ``` diff --git a/sync/class.jetpack-sync-sender.php b/sync/class.jetpack-sync-sender.php index 70cb4a09c..cd65e62bd 100644 --- a/sync/class.jetpack-sync-sender.php +++ b/sync/class.jetpack-sync-sender.php @@ -205,6 +205,8 @@ class Jetpack_Sync_Sender { if ( function_exists( 'fastcgi_finish_request' ) && version_compare( phpversion(), '7.0.16', '>=' ) ) { fastcgi_finish_request(); } + error_log( 'Does this show up' )? + sleep(10); $checkout_start_time = microtime( true ); ``` Do something that should trigger a sync request. Such as triggering a full sync or doing updating a setting. Next try doing an api request to you site. Without the call to fastcgi_finish_request(); the api request should take at least 10 seconds to complete. With the call to `fastcgi_finish_request();` the request should be a lot faster. #### Proposed changelog entry for your changes: * Improves sync performance on PHP 7.0.16 and higher with fastcgi_finish_request enabled.

enejb · 2019-01-25T20:49:52Z

testing.php
plugin to help us test this new feature.

<?php
/*
Plugin Name: Testing async syncing
Version: 1.0
*/

add_filter( 'pre_option_jetpack_sync_settings_async_sender', '__return_true' );
add_filter( 'jetpack_sync_send_data', 'eb_testing_send_data_sleep_before', 4, 1 );
add_filter( 'jetpack_sync_send_data', 'eb_testing_send_data_sleep_after', 14, 1 );

// fakes slow sync
function eb_testing_send_data_sleep_before( $send ) {
	error_log( 'Sleeping....' . getmypid() );
	sleep( 10 );
	return $send;
}

function eb_testing_send_data_sleep_after( $send ) {
	error_log( 'Done Sending... '. getmypid() );
	return $send;
}

jeherve · 2019-01-28T13:53:38Z

Is this ready for review?

kraftbj · 2019-04-17T21:53:00Z

Just checking on the expected timeline for this? Are we hoping to land it soon or could close if we're not going to add it?

lezama · 2019-04-18T14:16:15Z

@enejb what do you think about this one?

lezama · 2019-12-06T19:34:33Z

came up with a snippet we can use inside a plugin:

if ( isset( $_POST['action'] ) && 'jetpack_sync_async_sender' === $_POST['action'] ) {
	add_filter( 'jetpack_sync_sender_should_load', '__return_true' );
	add_action( 'wp_ajax_nopriv_jetpack_sync_async_sender', '__return_true' );
	ignore_user_abort( true );
} else {
	if ( ! ( defined( 'DOING_CRON' ) && DOING_CRON ) ) {
		add_action( 'shutdown', function () {
			if ( ! empty( \Automattic\Jetpack\Sync\Actions::get_sync_status()['queue_size'] ) ) {
				$args = array(
					'body'      => array(
						'action' => 'jetpack_sync_async_sender',
					),
					'timeout'   => 0.01,
					'blocking'  => false,
					'sslverify' => apply_filters( 'https_local_ssl_verify', false )
				);
				wp_remote_post( admin_url( 'admin-ajax.php' ), $args );
			}
		}, 10000 );
	}
}```

kraftbj · 2021-03-30T22:38:40Z

Going to close this but @mdbitz or @bisko, feel free to bring this back from the dead if so desired.

mdbitz · 2021-04-05T14:14:41Z

Thanks Kraft, agreed this can be closed and we are referencing it from Asana in the backlog for when we circle back to it.

lezama added [Package] Sync [Status] In Progress [Team] Poseidon labels Jul 18, 2017

lezama self-assigned this Jul 18, 2017

lezama requested review from gravityrail, westi, ebinnion and roccotripaldi July 18, 2017 17:56

lezama commented Jul 18, 2017

View reviewed changes

sync/class.jetpack-sync-actions.php Outdated Show resolved Hide resolved

allendav mentioned this pull request Jul 18, 2017

Disable Jetpack Sync during Shutdown for REST API Requests woocommerce/wc-api-dev#27

Closed

jeherve removed the [Team] Poseidon label Oct 18, 2017

brbrr added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Jun 26, 2018

stale bot added the [Status] Stale label Sep 24, 2018

lezama added 2 commits January 14, 2019 15:00

allow to load the sender async

f7f04e6

use async_sender Sync Setting

f64c350

lezama force-pushed the update/sync-async-sender branch from c794b98 to f64c350 Compare January 14, 2019 18:30

lezama requested a review from a team as a code owner January 14, 2019 18:30

stale bot removed [Status] Stale labels Jan 14, 2019

enejb mentioned this pull request Jan 15, 2019

Sync: add fastcgi finish request to sync #11146

Merged

lezama added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jan 17, 2019

lezama removed the [Status] In Progress label Jan 17, 2019

lezama added this to the 7.0 milestone Jan 17, 2019

enejb reviewed Jan 18, 2019

View reviewed changes

sync/class.jetpack-sync-actions.php Outdated Show resolved Hide resolved

enejb reviewed Jan 18, 2019

View reviewed changes

sync/class.jetpack-sync-actions.php Show resolved Hide resolved

lezama commented Jan 23, 2019

View reviewed changes

jeherve added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 24, 2019

lezama modified the milestones: 7.0, 7.1 Jan 29, 2019

jeherve removed this from the 7.1 milestone Feb 19, 2019

lezama force-pushed the update/sync-async-sender branch from bb6c2b7 to f64c350 Compare March 22, 2019 21:13

Automattic deleted a comment from broknowledge97 Dec 13, 2019

kraftbj removed request for a team, westi and roccotripaldi April 28, 2020 16:12

enejb mentioned this pull request Aug 25, 2020

Sync: Improve theme support syncing #16827

Merged

kraftbj closed this Mar 30, 2021

kraftbj removed the [Status] In Progress label Oct 8, 2021

xristos3490 mentioned this pull request Dec 29, 2022

Cleaning up the Bridge, Pt II Automattic/wc-calypso-bridge#885

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync: Add Async Sender #7482

Sync: Add Async Sender #7482

lezama commented Jul 18, 2017 •

edited

enejb commented May 2, 2018

lezama commented May 4, 2018

enejb commented May 4, 2018

lezama commented May 7, 2018

stale bot commented Sep 24, 2018

jetpackbot commented Jan 14, 2019 •

edited

enejb commented Jan 14, 2019

enejb commented Jan 22, 2019

lezama Jan 23, 2019

enejb Jan 23, 2019

enejb commented Jan 25, 2019 •

edited

jeherve commented Jan 28, 2019

kraftbj commented Apr 17, 2019

lezama commented Apr 18, 2019

lezama commented Dec 6, 2019

kraftbj commented Mar 30, 2021

mdbitz commented Apr 5, 2021

Sync: Add Async Sender #7482

Sync: Add Async Sender #7482

Conversation

lezama commented Jul 18, 2017 • edited

enejb commented May 2, 2018

lezama commented May 4, 2018

enejb commented May 4, 2018

lezama commented May 7, 2018

stale bot commented Sep 24, 2018

jetpackbot commented Jan 14, 2019 • edited

enejb commented Jan 14, 2019

enejb commented Jan 22, 2019

lezama Jan 23, 2019

Choose a reason for hiding this comment

enejb Jan 23, 2019

Choose a reason for hiding this comment

enejb commented Jan 25, 2019 • edited

jeherve commented Jan 28, 2019

kraftbj commented Apr 17, 2019

lezama commented Apr 18, 2019

lezama commented Dec 6, 2019

kraftbj commented Mar 30, 2021

mdbitz commented Apr 5, 2021

lezama commented Jul 18, 2017 •

edited

jetpackbot commented Jan 14, 2019 •

edited

enejb commented Jan 25, 2019 •

edited