Create "WP Heartbeat" API

[azaozz]

The purpose of this API is to simulate bidirectional connection between the browser and the server. Initially it will be used for autosave, post locking and log-in expiration warning while a user is writing or editing.

The idea is to have a relatively simple API that sends XHR requests to the server every 15 seconds and triggers events (or callbacks) on receiving data. Other components would be able to "hitch a ride" or get notified about another user's activities.

In the future this can be used to block simultaneous editing of widgets and menus or any other tasks that require regular updates from the server.

[azaozz]

I know, ​WebSockets are meant for this. Unfortunately few servers support them.

[nacin]

WebSockets use port 80, so it would simply be a matter for us to code it.

PHP 5.3 examples: ​https://github.com/nicokaiser/php-websocket and ​http://socketo.me/

[rmccue]

Replying to nacin:

WebSockets use port 80, so it would simply be a matter for us to code it.

PHP 5.3 examples: ​https://github.com/nicokaiser/php-websocket and ​http://socketo.me/

These need to be long-running processes (they're essentially separate servers on that socket), which may get killed by services internally. Long-polling (as per azaozz's suggestion) is a better idea given the variability of servers.

[bobbravo2]

I played around with that library this morning - and can tell you that both client & server implementations are lacking. I spent about 2 hours hacking with both - and was unable to get chrome or Firefox to establish a connection. I ended my exploration thinking that AJAX / transients are the way to go.

[bobbravo2]

Replying to rmccue:

Replying to nacin:

WebSockets use port 80, so it would simply be a matter for us to code it.

PHP 5.3 examples: ​https://github.com/nicokaiser/php-websocket and ​http://socketo.me/

These need to be long-running processes (they're essentially separate servers on that socket), which may get killed by services internally. Long-polling (as per azaozz's suggestion) is a better idea given the variability of servers.

I agree - and most Webhosts will kill any process around 180 seconds.

[knutsp]

Very interesting. How does the document editor on Google do simultaneous editing?

[bobbravo2]

Replying to knutsp:

Very interesting. How does the document editor on Google do simultaneous editing?

I was thinking the same thing - Google docs does multi user simultaneous editing VERY well. They have per-line locking, meaning that multiple users can edit the same doc @ the same time. I have played around with G'Docs before - trying to cause collisions and errors. The only time I remember being able to cause an error was by explicitly trying to edit the same line that I could see someone else was also editing. They handle that type of error by reverting your changes - and in real time - to minimize potential data loss.

In regards to implementing such a feature in WordPress - It would require both the server / client architecture that we're discussing here, as well as a separate patch / plugin for TinyMCE. Anyone have experience with the text parsing / event architecture of the TinyMCE API? I've written several plugins for TinyMCE - but those are used to extend it - and are not involved with the innards of the editor like this type of plugin will be.

[azaozz]

Concurrent editing is not on the table, yet :)

It cannot be done with a TinyMCE plugin. To be able to lock rows (or even paragraphs) TinyMCE would most likely have to move away from using contenteditable and do direct DOM manipulation so only the currently edited row (text node) will be "unlocked" and the rest will be a standard, non-editable HTML.

As far as I know Google docs do XHR saves every second while the user is typing, and in addition do polling every 10-15 sec. We will be doing something similar but instead of saving to the server, we will be saving to the browser's local storage (see #23220).

[F J Kaiser]

Replying to azaozz:

We will be doing something similar but instead of saving to the server, we will be saving to the browser's local storage (see #23220).

I just read about this on ​the Mozilla site - article from May 2012. Aside from the statement "terrible performance", I wonder which browsers support this. WP has always cared about a lot of browsers and I doubt that IE7 has a proper equivalent of localStorage.

[azaozz]

Replying to F J Kaiser:

Could we move this discussion to #23220, ​replied there.

[bobbravo2]

Replying to azaozz:

Concurrent editing is not on the table, yet :)

It cannot be done with a TinyMCE plugin. To be able to lock rows (or even paragraphs) TinyMCE would most likely have to move away from using contenteditable and do direct DOM manipulation so only the currently edited row (text node) will be "unlocked" and the rest will be a standard, non-editable HTML.

Sounds like TinyMCE would have to implement this type of feature in it's core?

[TJNowell]

Rather than a strict 15 seconds, maybe Wordpress should send back a value specifying a suggested time for the client to wait until the next 'beat'. We can set it to 15 seconds by default, but if we know something will be available in 5 or 10 seconds, or if the server is under heavy load and to wait for 30 seconds.

It would provide a lot more flexibility in the future and opens the door to basic load management plugins.

[azaozz]

Replying to TJNowell:

Rather than a strict 15 seconds, maybe Wordpress should send back a value specifying a suggested time for the client to wait until the next 'beat'...

Yes, was thinking/testing something similar. Perhaps we can speed up the beat when user B performs an action that would need response from user A like opening a post that is already being edited by user A, etc.

We can add support for this on both sides, PHP and JS.

Generally we have three options:

Standard polling

• 15 seconds interval, simple to implement, simple to use and understand.
• Covers all current user cases, however can "feel" slow when taking over a post lock.
• Possibly add throttling to improve user experience.

Long polling

In essence it would look something like this on the PHP side (on the JS side this is a standard XHR that runs in recursion):

function wp_poll() {

	for ( $i = 0; $i < 10; $i++ ) {
		$val = apply_filters( 'wp_poll', array() );

		if ( ! empty($val) ) {
			echo json_encode($val);
			exit;
		}

		sleep(2);
	}

	echo '0';
	exit;
}

• Better user experience than standard polling as it will deliver notifications to the browser faster (speed is only limited by the time needed to do a XHR). Also can be used when/if we implement concurrent editing.
• Ties resources on the server. There will be one process per user that will be running almost constantly.
• Needs to poll the DB.
• May run into problems with caching in PHP.

Hybrid

Start with polling and switch to long-polling when needed instead of reduced interval (as described above). This still needs some time to switch both user A and user B to long-polling, but preforms better after that.

[dd32]

May run into problems with caching in PHP.

I think that's a pretty big con, WordPress currently works in the form of pageloads, most people are OK with a page using stale cached data it retrieved 10ms ago, or even 100ms ago, once we start to talk in the 10~15second range though, that can be a lot of stale cached data in an environment where you're trying to make things "instantanious".

[azaozz]

Yeah, long-polling would break the perception of pageloads as it "waits" in the same process for 10-15 sec. for something to change, polling the DB (or memcached) to detect that change.

For example apply_filters( 'wp_poll', array() ) cannot be used to do get_option('...') as once loaded, options are stored in the $alloptions global.

[dd32]

Exactly my point, it means we can no longer use our API's, or would be required to write code that bypasses our own API's, or deliberately flushed the local cache every loop iteration.

The last part (flushing local cache on loop iteration) would mostly take care of it actually, It would avoid the cache issues, and allow the poll to skip the time to load and parse WP files.
I do however, think that it may be best left to a plugin for initial implementation, and us focus on a solution that reduces resource impact upon the server - WordPress can use quite a bit of memory in a page load, and most shared servers might not like a few resource hungry processes running for 15x their normal lifespan.

[nacin]

WordPress can use quite a bit of memory in a page load, and most shared servers might not like a few resource hungry processes running for 15x their normal lifespan.

Definitely. We can get away with a quick XHR request every 15 seconds, but probably not a long-poll like this. It's clever, but probably too clever for now.

[azaozz]

The last part (flushing local cache on loop iteration) would mostly take care of it...

You're right, it will take care of most places we "cache" in PHP but not of constant inside classes and functions. So we may still run into problems with our APIs. Add to that the extra server resources and long-polling doesn't look very appealing.

Agree that we should do normal polling and make it easier for plugins to implement long polling. Speeding the "heartbeat", perhaps down to 5 sec. plus time taken by the XHR, would also improve the user experience/reduce waiting to take over a post lock by up to 10 sec.

[azaozz]

In 23216.patch: first-run. Still need to decide whether to do separate filters for each data set sent through heartbeat (can be confusing as the filter name will depend on the data) or one filter for any data.

Another (major) consideration is use of custom events in JS. Would be best to get our own API #21170 up to speed and use it.

[azaozz]

In 23355:

Heartbeat API: first run, see #23216

[ocean90]

Seeing wp.heartbeat.interval(5); - What should happen if there are more then one plugins which are changing the interval on a page? Which one will win? How should it be handled?

[azaozz]

Replying to ocean90:

Currently the last one to set the interval wins. This implements the idea to be able to speed up or slow down the "beat" when needed (comment:21). wp.heartbeat.interval() gets the current interval and wp.heartbeat.interval(5) sets it. So a plugin can do:

if ( wp.heartbeat.interval() > 10 )
    wp.heartbeat.interval(10);

The default interval can also be set when initializing with the 'heartbeat_settings' filter in PHP.

Been thinking how to best implement this speed up/slow down. Seems the most common case would be to speed up when we expect certain response from the server, like while taking over a post lock: we need to notify user B and unlock the post for editing as soon as possible after user A releases the lock.

In that terms wp.heartbeat.interval() should probably accept two int arguments, first is the interval in seconds, second would be for how many "beats" (optional, default 10?). Then we can protect a lower value for the interval so plugins don't stomp each other.

Another possibility is to have three values: fast(5), normal(15), slow(30). Then setting it to slow will only be possible if the current value is normal, and setting fast will look into the second argument: for how long/how many "beats", and will revert to normal after that.

[azaozz]

The first run uses the JS global pagenow to identify from which screen the XHR was sent. That is different from the PHP global $pagenow (in JS it is $current_screen->id;). Will rename it to avoid confusion.

Another consideration: if the user has two tabs open with the same admin screen, on the PHP side they all look the same. We can detect when a tab is "blurred" and throttle the requests, but we may still run into problems if several tabs are open (more than 10?). Should we make the "beat" very slow, perhaps every 2 min, or even pause it if the user is not on the browser tab?

[aaroncampbell]

I would say slow it down a lot, but don't stop it. For example, if you have a tab open in the background and lose a post lock there, it would be nice to come back to the appropriate message rather than waiting for the heartbeat to start back up.

[azaozz]

In 23382:

Heartbeat API: throttle down when the window looses focus or when the user is inactive, always send 'screen_id', change the interval settings to 'fast' (5sec), 'standard' (15sec) and 'slow' (60sec), the interval can be changed from PHP, see #23216

[azaozz]

Small plugin to demo and test the first run.

[azaozz]

In [23382] there are two additions: detect when the window looses focus and when the user is not present. The first is looking at $(window).blur() and the second is looking for keyboard and mouse activity every 30 sec. Both ways also temporarily attach events to all iframes on the screen.

On detecting window blur or no user activity the interval is slowed down to 120 sec. Currently the "no activity" timeout is 5 min, so even if the window is in focus but the user doesn't type or move the mouse it will still slow down to 120 sec.

All this would need more testing and perhaps adjusting. There is some debug code in [23382] that outputs to the browser console. To turn that on, type or paste

wp.heartbeat.debug = true

in the console and run it on the Edit Post screen, or install heartbeat-test-plugin.php and test on the Dashboard.

[SergeyBiryukov]

Seeing the error in IE 8 on Edit Post screen in trunk:

Message: Could not delete 'window.heartbeatSettings'
Line: 34
Char: 4
Code: 0
URI: http://trunk.wordpress/wp-includes/js/heartbeat.js?ver=3.6-alpha-23386

[azaozz]

In 23389:

Heartbeat API: fix error in IE < 9, props SergeyBiryukov, don't attempt to bind events to cross-domain iframes, see #23216

[azaozz]

In 23481:

Heartbeat API: add nopriv actions, add JS 'heartbeat-send' event, see #23216

[azaozz]

In 23482:

Fix typo in wp_heartbeat_settings, see #23216

[markoheijnen]

/wp-includes/js/heartbeat.min.js is still missing.

[SergeyBiryukov]

Replying to markoheijnen:

/wp-includes/js/heartbeat.min.js is still missing.

Added in [23616].

[cbunting99]

Replying to azaozz:

The purpose of this API is to simulate bidirectional connection between the browser and the server. Initially it will be used for autosave, post locking and log-in expiration warning while a user is writing or editing.

So does this mean that Wordpress will be implementing a database sync framework? With a web based publishing platform, how are you going to save data locally to be synced with the server upon reconnecting? Since most Wordpress sites are not multi-server setups, how is the heart-beat going to work on a "Single Point of Failure "if the server crashes, storm takes out internet ect?

I could understand such a system if you were running ManageWP.com, they could check for connection loss and re-connection since their system (acting as a middle man) could then sync the data. I just don't see or can understand any benefit to adding this stuff if it can't do any more than what the current system does.. Unless of course you are going to re-write the admin utilizing something like ​http://appjs.org/ but I would doubt it. Am I missing something?

In regards to the future plans, Why not do something like most Wiki's and set a "being edited" flag.. You can show a notice to another editor that the article is currently being written/edited.. And or use ajax like live chat clients letting each editor see that the current document is being editing by such and such, once document is saved, give the green light to whos next. When the document is saved or canceled, remove the "being edited" flag.

[SergeyBiryukov]

Replying to cbunting99:

With a web based publishing platform, how are you going to save data locally to be synced with the server upon reconnecting?

Related: #23220

You can show a notice to another editor that the article is currently being written/edited..

Related: #23312

[azaozz]

In 23692:

Logged out warnings, heartbeat: remove nopriv_autosave as it doubles the functionality of the logged out warnings, move wp_ajax_nopriv_heartbeat() under No-privilege Ajax handlers in ajax-actions.php, see #23295, see #23216

[azaozz]

In 23882:

Heartbeat: improve setting the errorstate, add ajaxurl to the settings object when loading on the front-end, some code cleanup, see #23216

[DavidAnderson]

As I read the code on the original submission, the heartbeat runs always, regardless of whether it has any consumers using it. I'd suggest that it should only run if it has consumers - i.e., a more sophistcated API. If it does so, then it could potentially *reduce* server load (since all the parts of WP wanting a regular poll would ride onto the one heartbeat, instead of implementing their own). However if it runs *always*, even if not doing anything useful, it may be problematic.

If running always, then I'd suggest that some evaluation of performance impact is done for typical use cases. Otherwise, even if its impact is small, we may get more budget hosts saying "WordPress is a big performance hog", or more armchair tech pundits will start saying "don't run WordPress on a low-budget server".

What makes me suspect that it will have a real-world, measurable impact is that I have a plugin which if you open its admin page, it polls via AJAX to check the 'last log message' every 5 seconds. If I open two admin pages on localhost, then I can hear the CPU fa
n ramp up. CPU load goes from 0.40 (average when I'm just clicking around) to about 1.1 (average with 2 admin pages open with polling every 5 seconds). This is on a 1-year-old low-end laptop (T4500 dual-core, 4Gb, so no swapping is going on). So, not comparable to to proper web hosting, but possibly comparable to people who like to manage their own WordPress install and who buy a bottom-end VPS for it. This makes me think that it's non-trivial and that a proper, scientific evaluation would be valuable so that we'd know one way or the other whether it affects anyone or not. Based on my experience, a poll-every-15-seconds heartbeat means that every 1 admin page is adding about 0.2 to the average CPU load. So if a hosting company had 10 WordPress admin pages open at any one time on a server, they need an extra 2 CPUs, were they crazy and using Pentiums from low-budget laptops - which would be crazy of them; but you get the point - it's potentially a measurable affect.

David

[azaozz]

As I read the code on the original submission, the heartbeat runs always, regardless of whether it has any consumers using it.

Yes, currently 'heartbeat' is loaded everywhere in the admin and used by the login expiration warnings. Potentially we could be doing these warnings only on screens that need them most, like the Edit Post screen, all settings screens, etc.

Heartbeat runs on a 15 sec. interval only when the browser window is in focus and the user is active. If the window is "blurred" or there aren't any mouse and keyboard activities for 5 min, that interval drops to 2 min.

There will probably be some impact but as far as I see it's going to be insignificant. Not sure why your test "server" is performing so poorly. With the exception of the Edit Post screen, most users generally spend little time on most admin screens, a distant second is the Comments screen. So even if we load heartbeat only on the Edit Post screen (where it's most used atm) the impact wouldn't change.

[DavidAnderson]

Well, here's a crude estimation. On a 4-core Intel Xeon X3360 @ 2.83GHz webserver that I use, loading the WordPress dashboard (the server is using PHP via CGI) uses 0.3 seconds of CPU time (not wall time). On my laptop it's about double that. I've seen plenty of cheap VPSes that are comparable to my laptop.

Once very 15 seconds = one CPU can serve 50 WP admin logins on the Xeon, or one CPU can serve 25 WP admin logs on a cheaper setup. That seems like significant enough to take into account, or at least do more investigation into.

For example, we know that many web hosts disable loopback connections - pointless, annoying, but they do it. If word gets round that you can spare your CPUs + get more out of your servers by blocking heartbeat connections ("and all that happens is that WordPress users don't see login warnings, or lose some advanced functionality"), that'll not be fun. I wonder if something like login expiration warnings is worth taxing CPUs for every 15 seconds; or at least, the algorithm needs more attention. For example, login-expiration time is known when the page loads. Wouldn't it make more sense to just re-poll that once a minute before the known time rather than ask the question every 15 seconds? Rather than having a heartbeat every 15 seconds regardless of demand, why not have a heartbeat only when demanded by a consumer? (And for login expiration warnings, why not just use a one-off JavaScript setTimeout rather than polling it anyway?).

[PaulGregory]

@DavidAnderson You seem to be ignoring the fact that when you load admin pages from localhost on your laptop, your CPU is running the browser as well as the server. Indeed your laptop is most likely running all sorts of things that a VPS does not run. I think it is reasonable that WordPress can assume that everyone's hosting can cope with more than 4 pageloads/heartbeats a minute. Any hypothetical 'It's the apocalypse and CPU cycles are expensive' ration-time poster like "Is Your Heartbeat Strictly Necessary?" would be right alongside "Think Before You Click That Thing Again So Soon!"

You can rest assured that blocking heartbeat connections would be a much more targeted and deliberate act for a host to do than disabling loopback connections. Besides, the possibility of something being stopped is a poor reason not to start something.

[DavidAnderson]

@PaulGregory,

The 0.3 of CPU time was only measuring the CPU usage due to the PHP CGI process, not of any other processes. Or, if you're referring to the laptop running hotter (no swapping, recent model), then you're ignoring the fact that that's still a real-world effect. People will file bug reports: "Hey, my laptop fan goes every time I log into my WordPress development site dashboard. Why does it do that? It never used to on 3.5!". They'll want a reply that does more than reference imaginary posters. What I'm saying is not "Hey, stop that, bad idea!", but that a more thorough evaluation of the likely effects - *or lack thereof* - would be potentially valuable; and consideration of whether some things slated for the "heartbeat" (e.g. logout warning) would be better achieved a less resource-intensive way. To give another real-world example, I lived in Africa for 5 years. Telling a website developer on a 6-year-old laptop that "yes, heartbeat is bringing your laptop to a grinding halt, but that's life in 2013! Get a new laptop!" is all the same as telling him "don't use WordPress, it's not for you!".

David

[SergeyBiryukov]

Replying to DavidAnderson:

I wonder if something like login expiration warnings is worth taxing CPUs for every 15 seconds; or at least, the algorithm needs more attention.

Related: ticket:23295:44

[SergeyBiryukov]

In 24139:

Fix typo in heartbeat.js. see #23216.

[katzwebdesign]

This was really slowing down my localhost installation and I'm worried about the affect it will have on on page responsiveness. Partly the issue was because of resolving the DNS of localhost, but it was also taking too much CPU in general.

Here's an article I wrote on ways to slow down the interval or remove the heartbeat altogether: ​http://www.seodenver.com/wordpress-heartbeat/

[azaozz]

Perhaps the high CPU usage was caused by the DNS lookups? When properly set, the localhost DNS lookups should be virtually instant as they come from the hosts file. That may be the reason I don't see any significant change when I set the interval from 15 to 60 sec.

[azaozz]

In 24268:

Heartbeat: don't connect when a response is not expected by any script (nothing to send), remove debug logging, clean up code formatting, see #23216

[jtsternberg]

I have the debug log enabled and have been using trunk while testing a plugin of mine in development. It creates posts during an ajax action, and I've found that my debug log is filling up with the following errors:

[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1070
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1074
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1076
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1077
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1077
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1080
[20-May-2013 01:41:07 UTC] PHP Notice:  Trying to get property of non-object in /wp-includes/capabilities.php on line 1080

I put some debug things (debug_backtrace() written to the log) in place to see what was happening and traced it back to the new heartbeat ajax action. For some reason the actual post ID is never getting sent.

Once the error starts, it continues to write the same 7 error lines each time the heartbeat action happens (every 15-60 seconds?). Needless to say, this fills up my debug log pretty quickly.

Hopefully it will help you if I provide my debug log (with my own debug data injected) here. ​http://b.ustin.co/1ZYp

It's very possible I'm doing something wrong here, but nothing stands out to me, and it's definitely related to the new heartbeat api.

[azaozz]

Thanks for the detailed description and logs. Happens when there are no posts listed in the list table on the Posts screen. Patch coming up.

[azaozz]

Fixed in [24299].

[jtsternberg]

Replying to azaozz:

Thanks for the detailed description and logs. Happens when there are no posts listed in the list table on the Posts screen. Patch coming up.

Awesome, thanks! Always glad to know I'm not crazy or breaking things.

[nacin]

azaozz, this try/catch still results in a warning in the console on every heartbeat. Way around that?

		function isLocalFrame(frame) {
			try {
				if ( frame.contentWindow.document )
// Blocked a frame with origin "https://make.wordpress.org" from accessing a frame with origin "https://jetpack.wordpress.com". Protocols, domains, and ports must match.
					return true;
			} catch(e) {}

			return false;
		}

[SergeyBiryukov]

Just noticed that wp_heartbeat_settings() is currently located in wp-includes/general-template.php. Since it's not a template tag, would it make sense to move it to wp-includes/functions.php, near wp_auth_check_load()?

[toscho]

The PHP parts of this API are spread over 7 files currently. How about keeping those functions together in a single file if possible?

[azaozz]

Replying to nacin:

azaozz, this try/catch still results in a warning in the console on every heartbeat.

Happens only in Chrome. It ignores the try/catch completely (doesn't catch the error). We can try doing a string comparison between document.location and iframe.src, however that won't catch cases where the iframe starts as local and later a remote document is loaded in it.

[azaozz]

In 23216-2.patch: in isLocalFrame() compare window.location.origin to iframe.src to stop most cases where WebKit throws un-cachable exception about different iframe origin.

[nacin]

Looking over the ajax action handlers:

• Can we use some kind of separator for things like screenid and servertime? It looks sloppy.

• Can we do a convert_to_screen() and pass an actual WP_Screen object to the action, rather than just $screen_id?

• $data['user_id'] should just equal get_current_user_id().

• If no screen, we should use 'front' (per WP_Screen convention), not 'site'.

I'd still like to split up heartbeat_received into individual actions. Can we just loop through the events?

[azaozz]

Sure, patch coming up.

Thinking we can drop setting $data['user_id']. If a handler needs it it can get_current_user_id() easily (and it's cached).

We can do a convert_to_screen() but not sure if it will be worth it. The $screen_id is mostly used as an identifier for where the current request comes from so handlers can choose to run only on data coming from a particular screen.

[azaozz]

In 24384:

Heartbeat: in isLocalFrame() compare window.location.origin to iframe.src to stop most cases where WebKit triggers errors about different iframe origin, see #23216

[azaozz]

23216-3.patch changes/adds separator to the var names.

Thinking more about splitting heartbeat_received into separate filters. Something like this should work:

if ( ! empty($_POST['data']) ) {
	foreach ( (array) $_POST['data'] as $handle => $value ) {
		$handle = sanitize_key( $handle );
		$data = apply_filters( 'heartbeat-' . $handle, null, $value, $screen_id );

		if ( null !== $data )
			$response[$handle] = $data;
	}
}

The thing is there are four filters, two in PHP and two in JS. Splitting only one of them can make this feel inconsistent.

[alexvorn2]

I think that we can implement an additional feature for receiving data from the server as soon as it was modified or updated depending if the page was closed or not.

Simulation:
After the page will be loaded, the server will check if the tab is not closed every 30-40 seconds, if it was closed it will stop checking, else if the page is not closed the server will send data if needed as soon it happens.

Like on Facebook you get notifications as soon someone posted a comment...

[azaozz]

Replying to alexvorn2:

I think that we can implement an additional feature for receiving data from the server as soon as it was modified or updated depending if the page was closed or not.

Not exactly sure what you mean by "if the page was closed or not" but the rest is pretty easy to do. After a page loads, we poll the server at a regular interval and run couple of filters: if there is any data coming from the page, 'heartbeat_received' so all handlers (callbacks) can get data intended for them, then 'heartbeat_send' so any kind of data can be pushed from the server to the current page.

Have a look at the attached ​test plugin for an example. You can do pretty much anything in a function similar to heartbeat_test_comments_count() and have some JS on the page to receive the result and do something.

Also note that heartbeat won't connect to the server unless there is a script on the page that is expecting a response. This is to limit server load. So for screens where there is no core script using it, you will want to add something with $(document).on( 'heartbeat-send.my-script', function( e, data ) {...});.

[azaozz]

23216-4.patch​ extends 23216-3.patch​; wp-check-locked is renamed to wp-check-locked-posts, removes limiting of lock checking only to posts, limit initial setting of the interval to 15 - 60 sec.

[azaozz]

In 24406:

Heartbeat: rename some vars/args to make them more intuitive, don't set user_id on every request, see #23216

[azaozz]

In 24528:

Nonce refresh:

• Update the heartbeat nonce when refreshing nonces on the Edit Post screen.
• After a user logs in from the auth-check dialog, speed up heatrbeat to check/refresh nonces on the Edit Post screen.
• Speeding up heartbeat: bring back the setting how long it should last (how many ticks).
• Add 'heartbeat-nonces-expired' jQuery event when nonces have expired and the user is logged in.

See #23295, see #23216.

[azaozz]

In 24577:

Heartbeat:

• Pass the actual error to the jQuery event 'heartbeat-connection-lost'.
• When in error state, keep trying to connect even when no response is expected until the error is cleared.

See #23216.

[azaozz]

This is feature completed. Any bugs should go in new tickers.

[carldanley]

After talking with nacin on IRC, I reviewed the status of the heartbeat JS file and made changes to improve the quality and performance. I've organized my changes into a 3.6 ship list. I honestly think that this whole patch should be shipped. Please let me know if you have any questions.

Can't ship without these, imo
=============================

• cleaned up a lot of code that was missing semi-colons and other things
• explicitly declare exposed function calls for a clear idea of what is/isn't available to a user through public API methods
• moved the values of autostart and connectionLost to internal variables and created 2 new functions: shouldAutoStart() and _hasActiveConnection() respectively. See notes below about changing respective areas.
• fixed a lot of missing semi-colons from various areas
• added a layer of caching to various settings and objects used internally. This should speed things up a little.
• fixed an issue where timers could be potentially duplicated
• added rigorous notes and questions/comments to todo:'s
• moved a few random snippets of code (e.g.- things like L334, L282) into _initialize() function and other places that make more sense
• the use of brackets is preferred standard in JS and the current iteration was lacking a ton of brackets all over the place

Optional but really should be included
======================================

• removed a lot of unnecessary named function expressions which make debugging a little more difficult due to anonymity
• added a heavy set of code documentation
• changed the logic of the switch fall-through's defined in last iteration, specifically errorstate() which has now become _triggerError(). This was a private name change and doesn't affect anything in the API.
• moved random prototype extension on L463 to actual functions so things are consistent

Not too important but added while redoing everything else
=========================================================

• changed various internal variable names for clarity when reading code really put effort into this as a lot of things were pretty confusing previously
• added a few extra internal helper functions to enhance clarity while reading the code.
• renamed a few internal helper functions for clarity
• moved a few anonymous jQuery event callbacks into separate functions for clarity

[azaozz]

I would have really appreciated another pair of eyes on this couple of months ago, when heartbeat was in development. Restructuring/rewriting parts of it in RC seems too late.

Currently the API is in "experimental" stage. The usefulness of the initial idea was severely reduced by concerns it will overwhelm shared hosting accounts because it connects every 15 seconds while a user is logged in. It is structured as a (pseudo) class so it can have private and public vars and methods, however that class is intended to be initialized only once and is not intended to be extendable.

In that terms the _Cashe and _Settings objects in 23216-heartbeat-cleanup.diff​ ate pointless. If the concerns that connecting every 15 seconds is too much to handle for an average host are proven to be true, heartbeat would probably be reduced to couple of helper functions. If these concerns are proven wrong, heartbeat can be made into a "real" JS class (or couple of classes) that can be extended, and maybe support several instances running at the same time, etc.

Regarding coding standards:

• The use of curly brackets on if is optional in the WordPress coding standard. This is mostly a readability thing as the minimizer adds and removes brackets where needed, so the production (minimized) code is unchanged.
• A single space is required after if, for, while, etc.
• The else and else if should be on the same line as the preceding curly bracket.
• You mention this couple of times but I don't see any missing/added semicolons in 23216-heartbeat-cleanup.diff :)

Using switch () { } instead of a long list of if () { } else if () { } else if () { }, seems more readable. But this is a personal preference.

Wrapping a DOM node in jQuery is very fast (it bypasses all the css selectors logic, etc.). Not sure caching of $(document) and $(window) makes any significant difference but doesn't hurt. I suppose $(document) is as readable as $document for most people, _Cache.$document might not be.

Having a lot of inline comments is nice, explaining too much may feel a bit strange though:

// set the value of hasFocus to false 
 _Settings.hasFocus = false; 

Like the function names changes, not sure all functions in the private scope have to start with an underscore. Don't see this anywhere else in WordPress. Also not sure it needs all the new private functions. Maybe they make heartbeat a little bit slower after the patch.

[carldanley]

Replying to azaozz:

I would have really appreciated another pair of eyes on this couple of months ago, when heartbeat was in development. Restructuring/rewriting parts of it in RC seems too late.

I do apologize for not being available earlier. This was my first available time, and Nacin suggested that I look at this ticket. I hope you will find my revised contribution tomorrow morning to be helpful even at this late stage. I will be taking all of your comments into consideration as I prepare it.

Currently the API is in "experimental" stage. The usefulness of the initial idea was severely reduced by concerns it will overwhelm shared hosting accounts because it connects every 15 seconds while a user is logged in. It is structured as a (pseudo) class so it can have private and public vars and methods, however that class is intended to be initialized only once and is not intended to be extendable.

I agree that iteration and experimentation are a great way to push forward. Hopefully my contributions now will help all of us in later iterations. The code in the patch I supplied is also structured as a module with public and private variables & methods as well; this was intended so the patch matched the modular approach that is currently in place.

In that terms the _Cashe and _Settings objects in 23216-heartbeat-cleanup.diff​ ate pointless. If the concerns that connecting every 15 seconds is too much to handle for an average host are proven to be true, heartbeat would probably be reduced to couple of helper functions. If these concerns are proven wrong, heartbeat can be made into a "real" JS class (or couple of classes) that can be extended, and maybe support several instances running at the same time, etc.

_Cache and _Settings are a much more organized approach to clearly defining variables into an appropriate namespace. _Cache and _Setting were intended only to make the current code easier for other developers to understand so that they can contribute in later iterations. I think there was a lot of confusion in the 17 line long variable declaration with no prefix. With _Cache and _Settings we now know what the purpose of each privately scoped module variable is and where it belongs. When reading through the current iteration it was really difficult to discern the difference between global module variables and function variables.

Regarding coding standards:

• The use of curly brackets on if is optional in the WordPress coding standard. This is mostly a readability thing as the minimizer adds and removes brackets where needed, so the production (minimized) code is unchanged.

Even though it gets minimized, it's still important that we maintain proper standards going forward. This style is largely based off of both: ​http://make.wordpress.org/core/handbook/coding-standards/javascript/#ifelse and ​http://contribute.jquery.org/style-guide/js/#spacing and is something that is common advice throughout most modern JS frameworks.

• A single space is required after if, for, while, etc.
• The else and else if should be on the same line as the preceding curly bracket.

Thank you for those notes. I will patch accordingly.

• You mention this couple of times but I don't see any missing/added semicolons in 23216-heartbeat-cleanup.diff :)

Mostly because I removed the use of NFE's which lead to anonymous debugging. Here are a few examples of missing semi-colons in the current iteration of heartbeat.js:

• L208 - unnecessary semi-colon after function declaration
• L347, 423, 450, 461 - missing semi-colon after named function expression variable declaration

Using switch () { } instead of a long list of if () { } else if () { } else if () { }, seems more readable. But this is a personal preference.

I agreed that the transition from switch to if...else if...else if... is not important. However, after researching a lot of different use-cases for switches, it seems like I'm sold on avoiding them when possible due to memory leaks in older browsers. You can thank Douglas Crockford for my dislike of switch statements =P

Wrapping a DOM node in jQuery is very fast (it bypasses all the css selectors logic, etc.). Not sure caching of $(document) and $(window) makes any significant difference but doesn't hurt. I suppose $(document) is as readable as $document for most people, _Cache.$document might not be.

I tried to find the jsperf test that demonstrates the speed benefits of caching these (or any) jQuery selectors and I couldn't find it off the bat BUT this test will do for now: ​http://jsperf.com/jquery-window-query-caching2 . I can guarantee you can run it in any browser and the cached value will be *much* higher. That specific test isn't too saturated but I'm positive of the benefits in this simple caching routine and that this will speed up the module just a tad.

Having a lot of inline comments is nice, explaining too much may feel a bit strange though:

// set the value of hasFocus to false 
 _Settings.hasFocus = false; 

Good point, I hadn't intended to leave those in. I will remove.

Like the function names changes, not sure all functions in the private scope have to start with an underscore. Don't see this anywhere else in WordPress. Also not sure it needs all the new private functions. Maybe they make heartbeat a little bit slower after the patch.

I feel that prefixing with an underscore is a great convention and try to promote it as a good way to clearly denote the use of private methods, but yes it is a preference.

As I mentioned, I will send a revised version tomorrow and let you take a look through it. Hopefully you will find parts of it to be helpful. It is totally up to you which parts you decide to keep.

[carldanley]

I've uploaded the new revision of the patch with the following changes from your recommendations:

• single spaces after if, for, while, etc
• moved else and else if's to same line
• removed a few redundant comments

Please let me know if you see anything else that I can improve.

[adamsilverstein]

Replying to carldanley:

I've uploaded the new revision of the patch with the following changes from your recommendations:

• single spaces after if, for, while, etc
• moved else and else if's to same line
• removed a few redundant comments

Please let me know if you see anything else that I can improve.

Note that the WordPress JavaScript coding standards also call for preferring single quotes over double quotes., unless required - ​http://make.wordpress.org/core/handbook/coding-standards/javascript/#quotations

[adamsilverstein]

replace double quotes with single quotes as per coding standards

[carldanley]

Great point! Glad you worked that in; got my standards mixed up for a second: ​http://contribute.jquery.org/style-guide/js/#quotes.

[adamsilverstein]

Replying to carldanley:

Great point! Glad you worked that in; got my standards mixed up for a second: ​http://contribute.jquery.org/style-guide/js/#quotes.

yes, its easy get mixed up, but posting to trac is a sure way to get corrected :) (i know from experience)

to azaozz's credit, i think the suggestion to always use curly brackets for conditionals in JS was added sometime after he wrote the bulk of that code. I might be wrong, but I'm pretty sure it used to read more like the PHP spec which suggests omitting them for brevity, ​http://make.wordpress.org/core/handbook/coding-standards/php/#brace-style.

I agree its better to use them (and Douglas Crockford explains why).

after installing your patch (i went pack to your version to be sure it wasn't my quote change), i am getting the following error in the JS console, and my heart has stopped beating:

TypeError: _Settings.hasActiveConnection is not a function
[Break On This Error] 	

if ( _Settings.hasActiveConnection() === false ) {

heartb...1-24717 (line 376)

[wonderboymusic]

the Revealing Module Pattern is one of the only ways to actually make methods actually private, would also delete the need for dangling _, which every linter complains about - otherwise, nothing stops API._imPrivate from being called, this would:

var JS_Class = (function () {
    function privateFunc() {}

    function publicFunc() {
         privateFunc();   
    }
  
    return {
	publicFunc : publicFunc
    };
}());

OR

(function () {
  function unreachable() {
		
  }
	
  function JS_Class() {
    function whatever() {
       unreachable();
    }
  }
	
  window.JS_Class = JS_Class;
}());

OR

var JS_Class = (function () {
    function privateFunc() {}
    return {
	 publicFunc : function () {
              if (window.authorized) 
	          privateFunc();   
	
	       doOtherstuff();
	 }
    }
}());

I think it is a little absurd that a rewrite is even being entertained during RC.

[carldanley]

Replying to adamsilverstein:

Replying to carldanley:
after installing your patch (i went pack to your version to be sure it wasn't my quote change), i am getting the following error in the JS console, and my heart has stopped beating:

TypeError: _Settings.hasActiveConnection is not a function
[Break On This Error] 	

if ( _Settings.hasActiveConnection() === false ) {

heartb...1-24717 (line 376)

Excellent, will patch ASAP.

[carldanley]

Replying to wonderboymusic:

the Revealing Module Pattern is one of the only ways to actually make methods actually private, would also delete the need for dangling _, which every linter complains about - otherwise, nothing stops API._imPrivate from being called, this would:

This is in fact the pattern I used (​http://addyosmani.com/resources/essentialjsdesignpatterns/book/#revealingmodulepatternjavascript). I think what you're suggesting is the removal of the underscore prefixes which are only there as an added benefit in reading the code. Underscore prefixes can be removed during minification which is why I believe there is added value in the prefix for readability. I'm open to the removal of the underscores as it's an easy change.

I think it is a little absurd that a rewrite is even being entertained during RC.

If I were to rewrite this code, I would have started from scratch with a different approach. I mostly copied and pasted code and cleaned up a lot of pieces to match a more solid structure. I also agree that is indeed late in the game but I don't think it would be hard to entertain this given the API matches and things work the same as they did before.

[carldanley]

adamsilverstein - Please use the updated patch and let me know if you find anything else.

[adamsilverstein]

Replying to carldanley:

adamsilverstein - Please use the updated patch and let me know if you find anything else.

same error. to get this i'm in firefox, i load edit post page, go to another program then switch back to the open window.

have to agree with wonderboymusic, seems to late to make any dramatic changes to the heartbeat code at this late stage (say nothing of the revisions work!).

perhaps you would consider submitting a patch that limits the changes to: adding brackets where missing, adding inline docs, function renaming where helpful to clear up purpose and bug fixes. leave out anything that complicates or changes the code structure for now and submit a new ticket for these updates for further consideration.

[carldanley]

The patch I just uploaded fixes all problems. I rechecked everything and things seem back to normal.

[carldanley]

The above patch can be combined with the 23216-cleanup-fixes.diff (​http://core.trac.wordpress.org/attachment/ticket/23216/23216-cleanup-fixes.diff ) to patch the small errors in this file. It removes the direct property access to autostart and connectionLost that could be difficult to deal with in the future for API reasons. I introduced the functions hasActiveConnection() and shouldAutoStart() instead so that we have flexibility going forward. I also patched the missing semi-colons in the current iteration.

This is a much smaller changeset aimed at 3.6. The other cleanup diff can be used for early 3.7 when it's not so late in the game.

[azaozz]

Thinking more about this: the start, stop and autostart presently don't make much sense. They were "maybe useful" for debugging and when heartbeat was connecting on each "tick", but since it skips now, there's no good reason to let it be paused or stopped from JS. Perhaps we should remove these for now and decide in a future iteration.

[carldanley]

I also agree with that. This might be better to always be "on" for now. Probably better to avoid having to support the extra API methods until we really do need them.

[azaozz]

@carldanley thanks a lot for contributing to this. It's true it's late in the current release cycle, probably best to concentrate on cleaning up/locking/removing things that are not essential.

Thinking we should start by "officially" declaring this API as experimental, meaning that it will likely change a lot in the next release or two (depending on feedback from 3.6). During the last release cycle there was a discussion about having some sort of "API maturity" indicator. Seems now is a good time to start.

In addition to 23216-small-heartbeat-changes.diff thinking we can add the function and var name changes. Perhaps instead of connectionLost or hasActiveConnection we can call that bit connectionError and/or hasConnectionError and drop the setter part/keep the var private.

For future iterations, few quick notes on 23216-heartbeat-cleanup.5.diff:

• The nonce has to be pulled from window.heartbeatSettings on every xhr so we can update it when it expires (only happens on the Edit Post screen for now).
• document.querySelectorAll() is missing in IE7 and (unfortunately) we still support it.
• The idea to wrap private vars in an object is cool. However (call me old-fashioned) obj.variable always suggests an unidentifiable scope at first look. (In my mind simple objects are usually associated with external scope or something like a mixin.) On the other hand it adds some clarity when dealing with several nested scopes. Perhaps we can start using this with the next iteration.
• Thinking we should keep the try...catch in isLocalFrame() to prevent any possible iframe origin exceptions.

[carldanley]

Awesome stuff. So for 23216-small-heartbeat-changes.diff, we are:

• incorporating the function and var names for clarity
• changing hasActiveConnection() to hasConnectionError() and removing the setter part. This variable will now be internal only

Did you still want to remove start, stop, & autostart for this 23216-small-heartbeat-changes.diff?

I'll focus on the bigger patch after we get this small one cleaned up and ready to ship. Great recommendations!

[azaozz]

Replying to carldanley:
Great.

Did you still want to remove start, stop, & autostart for this 23216-small-heartbeat-changes.diff?

Yes, the start/stop/autostart functionality is non-essential, best to take it out for now.

[carldanley]

Replying to azaozz:

Replying to carldanley:
Yes, the start/stop/autostart functionality is non-essential, best to take it out for now.

Awesome, will patch this up and send you a diff within an hour or two.

[carldanley]

Replying to azaozz:

Replying to carldanley:
Yes, the start/stop/autostart functionality is non-essential, best to take it out for now.

Please refer to this patch: ​http://core.trac.wordpress.org/attachment/ticket/23216/23216-small-heartbeat-changes.3.diff .

Contains:

• semi-colon fixes
• no more start()/stop() functionality
• autostart removed
• removed connectionLost property and introduced hasConnectionError()
• patched all files that accessed the properties that were removed (just autostart.js)

azaozz: take a look and just make sure everything looks ok.

[carldanley]

removed === equality checks for nacin

[azaozz]

23216-small-heartbeat-changes.4.diff works well, +1 to commit.

[nacin]

In 24749:

Simplify heartbeat API.

• Move to a method to check connection errors, a better framework for future changes.
• Remove start(), stop(), autostart.

props carldanley.
fixes #23216.

[carldanley]

added check for heartbeat in autosave

[carldanley]

Now that 3.6 is out, should we move my cleanup file to a new ticket and work on implementing for 3.7?

[azaozz]

Replying to carldanley:

Sure. Will also need feedback on whether some of the "moving parts" need adjustment: is 15 sec. too much to handle for most servers, can we remove the skipping so we have more consistent way to push from the server, etc.

It would be great to have #21170 in core and use applyFilters() instead of jQuery. Also switch over remote autosave to heatrbeat (after refreshing/refactoring).

[strangerstudios]

Fixed original test plugin.

[strangerstudios]

FYI, I fixed the original test plugin and uploaded a new one. (I couldn't replace the original.)

The issue was that the Heartbeat now won't return any data from the server if no data is passed from the client (the Heartbeat is never kicked off at all). So I added a section to the bottom of the JS that just sends any old thing to kick things off.

I also put together an alternative minimal example here that may be useful for people:
​http://www.strangerstudios.com/blog/2013/08/heartbeat-api-for-wordpress/

Cheers!

[jeffr0]

In response to the issues David Anderson brought up when the API was in initial development concerning server performance, InMotion Hosting did a nice writeup and experiment showcasing the performance impact of the Heartbeat API on their servers. ​http://www.inmotionhosting.com/support/website/wordpress/heartbeat-ajax-php-usage#heartbeat-in-action

There's also quite a few reports from HostGator customers in particular that routinely have their site suspended with POST /wp-admin/admin-ajax.php labeled as the culprit. I experienced this personally when I hosted a site on HostGator and upgraded to WordPress 3.6. I'm not sure how widespread of an issue it is today, but based on comments in this article ​http://wptavern.com/how-to-take-control-of-the-wordpress-heartbeat-api#comments it's still happening.

The problem is solved by either slowing down the Heartbeat API, moving to a host with more lax resource restrictions, or purchasing a VPS.

[azaozz]

Replying to jeffr0:

There were some improvements in 4.1: it "goes to sleep"/"wakes up" better in new browsers, and plugins can disable it easily or set minimal interval so it doesn't overload the server. See #29779 and [30293].

[lukecavanagh]

Is there not a way to define a value in seconds, instead of the hardcoded default of 15 seconds?

[soulseekah]

Replying to lukecavanagh:

Is there not a way to define a value in seconds, instead of the hardcoded default of 15 seconds?

Use the wp.heartbeat.interval function ​https://github.com/WordPress/WordPress/blob/703d5bdc8deb17781e9c6d8f0dd7e2c6b6353885/wp-includes/js/heartbeat.js#L597