Make WordPress Core

Opened 2 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#61960 closed defect (bug) (fixed)

Heartbeat API: consider changing the min allowed time

Reported by: annezazu's profile annezazu Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: performance Cc:

Description

In conversations with folks about phase 3 of WordPress compiled in Summary and Insights of Phase 3 related conversations , getting locked out of a post and dealing with pain points around residual locking continually came up. While waiting for more collaborative workflows to be built, improving the locking experience in the interim would be a big improvement to day to day workflows. I chatted with @jorbin who mentioned that the heartbeat can be sped up so it checks every 15 seconds as it was originally set here:

https://core.trac.wordpress.org/changeset/24406

Historically, it seems this was done over a decade ago due to performance concerns on shared hosts: https://core.trac.wordpress.org/ticket/23216#comment:57 Is there space to revisit this and move it back to the original 5 seconds? Rather than dealing with getting locked out "hundreds of times a day" as one recent interviewee said, we could offer a way to lower the lock out time and ease people's workflows today.

If a trac ticket has been open for this previously, I apologize! I did look around first before opening this one and appreciate being pointed to the right place as this does come up a fair amount when chatting with folks.

Attachments (1)

heartbeat-test.png (142.0 KB) - added by azaozz 6 weeks ago.

Download all attachments as: .zip

Change History (26)

#1 @jorbin
2 months ago

  • Milestone changed from Awaiting Review to 6.7

I like the idea of allowing for setting a quicker heartbeat, especially in light of the discussion in https://github.com/WordPress/gutenberg/discussions/65012 and to further collaborative editing.

While not every host is going to work well with a quick heartbeat which was the original reason for pushing it to 15, many hosts now can accommodate more requests now than they did 11 years ago.

Moving to 6.7 for visibility and to encourage discussion.

cc/ @kirasong @azaozz due to your knowledge and experience with heartbeat.

#2 @azaozz
2 months ago

heartbeat can be sped up

. ...

it seems this was done over a decade ago due to performance concerns

Sure, there doesn't seem to be a good reason to keep that imitation any more. As far as I see it can be removed completely (min = 1 second?) so it can be sped up when needed.

I'll be a bit hesitant to set the default to 5 seconds though. Heartbeat is used on all wp-admin screens to check for expiring user logins. That user case doesn't need frequent checks. Seems better to remove the limitation and speed it up when more than one user is writing/editing, but not change the default frequency.

Version 0, edited 2 months ago by azaozz (next)

#3 @peterwilsoncc
2 months ago

The authentication cookie is set to expire 12 hours after the login session expires (which is stored in the cookie itself), see source code.

This gives the heartbeat API plenty of time to prompt a user to log in when the session expires before the cookie expires.

I know there have been issues in the past with nonces expiring while posts are open in a background tab as browsers pause JavaScript execution. I don't know that allowing for faster refreshes of the heartbeat will solve that issue as they too rely on JavaScript.

However...

Increasing the cookie retention period to 24 hours may help partly resolve this. Typically there are 16 hours between someone wrapping up for one day and starting the next. If the session expires shortly after logging off, then the 12 hour grace period will expire before they return to their browser.

#4 @kirasong
2 months ago

@annezazu, thanks so much for the ticket!

I'm not opposed to reducing the time between the heartbeat requests, but I'm having a hard time understanding how to reproduce the issues that users are having.

Would you mind providing a specific example or two, please?

It's possible I missed them on the link you included, and apologies if so!

#5 @peterwilsoncc
2 months ago

Re-reading this, it looks like I got confused between types of locking. On a re-read I think this is referring to post locking rather than a user getting their account locked while away from their browser. Sorry about that.

#6 @annezazu
2 months ago

Would you mind providing a specific example or two, please?

Sure! I'll try to get some specific examples from some larger sites. The root issue is a large number of users working on many posts per day, including needing to jump in, review a post, then jump out. The issue is that there's often a huge lag when someone is in a post vs when they are seen as out of the post. Even without real time collaboration happening, improving this so there's less lag would be a big step in improving collaboration in general.

#7 @azaozz
2 months ago

The issue is that there's often a huge lag when someone is in a post vs when they are seen as out of the post.

The 15 seconds "default" for checking post locks is set from (old) JS: post.js and inline-edit-post.js. but doesn't seem to be set when the block editor is used so it runs on a 60 seconds interval (thought this was set there before). This can be set from JS or PHP, will make a quick patch.

This ticket was mentioned in PR #7301 on WordPress/wordpress-develop by @azaozz.


2 months ago
#8

  • Keywords has-patch added

Also set it to 10 seconds (down from 15) on the Posts screen so post locks can be set and removed faster.

Update Heartbeat: remove interval limitations; now the interval can be from 1 to 3600 seconds.

Trac ticket: https://core.trac.wordpress.org/ticket/61960

@azaozz commented on PR #7301:


2 months ago
#9

Hoping that web servers have somewhat evolved and became more powerful over the last 11 years :)

However if doing an AJAX request every 10 seconds while editing a post is still too much for some shared hosting, this can be reverted to the old setting of 15 seconds.

@azaozz commented on PR #7301:


2 months ago
#10

Should the classic editor's default interval be lowered too?

Sure, lets change it there too. Was a bit hesitant as it seems every little change on the (old) Edit Post screen could trigger something unexpected, and it is in a "deep maintenance" mode so few people would test it, but seems this should be okay.

@peterwilsoncc commented on PR #7301:


2 months ago
#11

Yeah, let's give the classic editor a go to. It's yolo Friday in your part of the world tomorrow so (pending @getsource's feedback) unless someone else picks up a blocker, maybe you can do the commit honours then so the hosting folks can offer feedback

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 weeks ago

#13 @peterwilsoncc
8 weeks ago

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

In 59016:

Administration: Increase frequency of heartbeat API requests.

Increases the frequency of heartbeat API requests from once every 15 seconds to once every 10 seconds.

The purpose of this change is to reduce the length of time before a post becomes unlocked as a user navigates around the WordPress Dashboard and ceases editing a post.

wp.heartbeat.interval() has been modified to allow theme and plugin authors to set the heartbeat interval to any value between one second and one hour rather than limiting them to a fixed set of values.

Props azaozz, annezazu, jorbin, kirasong.
Fixes #61960.

#15 follow-up: @peterwilsoncc
7 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as the commit introduced a JavaScript bug on the block editor.

Uncaught TypeError: wrap is undefined
    <anonymous> http://wp-build.local/wp/wp-includes/js/wp-auth-check.js?ver=6.7-alpha-59022:165
    jQuery 7
    xhr http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:479
    jQuery 8
    connect http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:436
    scheduleNextTick http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:536
    interval http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:777
    <anonymous> http://wp-build.local/wp/wp-admin/post-new.php:1824
wp-auth-check.js:165:41
    <anonymous> http://wp-build.local/wp/wp-includes/js/wp-auth-check.js?ver=6.7-alpha-59022:165
    jQuery 7
    xhr http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:479
    jQuery 8
    connect http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:436
    scheduleNextTick http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:536
    interval http://wp-build.local/wp/wp-includes/js/heartbeat.js?ver=6.7-alpha-59022:777
    <anonymous> http://wp-build.local/wp/wp-admin/post-new.php:1824

I suspect an order of operations issue and will investigate.

#16 follow-up: @peterwilsoncc
7 weeks ago

Replacing the code in src/wp-admin/edit-form-blocks.php with the following works:

<?php
// Set Heartbeat interval to 10 seconds, used to refresh post locks.
wp_add_inline_script(
        'heartbeat',
        'heartbeatSettings = heartbeatSettings || {}; heartbeatSettings.interval = 10;',
        'before'
);

@azaozz if possible I'd prefer to use the heartbeat_settings filter but adding it to src/wp-admin/includes/admin-filters.php caused the code to run too late. As an aside, this appears to affect wp_heartbeat_set_suspension() too.

#17 in reply to: ↑ 16 @azaozz
7 weeks ago

Replying to peterwilsoncc:

@azaozz if possible I'd prefer to use the heartbeat_settings filter but adding it to src/wp-admin/includes/admin-filters.php caused the code to run too late.

Uh, I see. Even if we use the (PHP) filter there should be some default value for the block editor's screen. I'm away next week (WCUS) but will find some time to debug this too.

#18 in reply to: ↑ 15 @azaozz
6 weeks ago

Replying to peterwilsoncc:

Reopening as the commit introduced a JavaScript bug on the block editor.

Been testing trunk for about 2 hours now, both the block editor and Posts screens, and don't see this error (testing in Firefox). Is is possible this was some sort of temp/caching related thing?

Thought this may have something to do with adding the code 'before' or 'after', possibly a script loader bug. But seems all runs well here, interval is 10 seconds and can see the "Please log-in again" modal popping up as soon as needed.

Would it be possible to test again and if still error(s), post the exact steps to reproduce.

@azaozz
6 weeks ago

#19 follow-up: @peterwilsoncc
6 weeks ago

@azaozz Thanks for looking in to it, I am still seeing the error.

The error is occurring for me upon opening a new post, so:

  1. Log in to WP Dashboard
  2. Click Add New Post

However, if I enable network throttling in dev tools the error doesn't show so it appears to be an order of operations issue. Possibly related to script dependencies needing an update??

When testing the package update in [59072] for #61906 @noisysocks was able to reproduce the issue too, so I suspect it has something to do with the speed of local environments.

This ticket was mentioned in PR #7429 on WordPress/wordpress-develop by @azaozz.


6 weeks ago
#20

Seems this should be running on DOM ready.

Trac ticket: https://core.trac.wordpress.org/ticket/61960

#21 in reply to: ↑ 19 @azaozz
6 weeks ago

Replying to peterwilsoncc:

The error is occurring for me upon opening a new post, so:
...
However, if I enable network throttling in dev tools the error doesn't show

Still doesn't happen here for some reason (testing in Firefox, Brave (Chromium), and Safari), but think I know what's going on. The Heartbeat interval setting script should be running on DOM ready, not as soon as loaded. Please see if https://github.com/WordPress/wordpress-develop/pull/7429 fixes it.

@azaozz commented on PR #7429:


6 weeks ago
#22

Are you able to add dom ready as a dependency to the heartbeat

Sure.

Changing this to after will negate the need to check heartbeat is defined

Right. Seems the before/after wouldn't make much difference as they will not be outputted if heartbeat.js is not outputted. But you're right, this patch/PR is way too cautious, was mostly to confirm it works now. It can be written better as both heartbeat.js and window.wp will be defined in all cases. Fixing..

@azaozz commented on PR #7429:


6 weeks ago
#23

Simplified and switched to using jQuery for DOM ready. Heartbeat depends on it already, seems no point in adding another dependency.

#24 @azaozz
6 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 59092:

Administration: Fix increasing of the frequency of Heartbeat API requests.

Props peterwilsoncc, azaozz.
Fixes #61960.

Note: See TracTickets for help on using tickets.