Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scheduling just one recurring action makes more than one #698

Open
Nemi5150 opened this issue May 19, 2021 · 11 comments · May be fixed by #830
Open

Scheduling just one recurring action makes more than one #698

Nemi5150 opened this issue May 19, 2021 · 11 comments · May be fixed by #830
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: bug The issue is a confirmed bug.

Comments

@Nemi5150
Copy link

On the Usage page there is an example of how to "schedule a function to run at midnight if it is not already scheduled". See here
https://actionscheduler.org/usage/

I have used this example to create an action that runs every minute, and only one gets scheduled. However, every once in a while I check on the scheduled actions and there are two of those running. My guess is that there must be a race condition two requests are running at the same time and end up scheduling two upcoming actions. They then stay in perpetuity.

What is the best way to keep this from happening?

@barryhughes
Copy link
Member

Thanks for flagging this, @Nemi5150.

My guess is that there must be a race condition two requests are running at the same time and end up scheduling two upcoming actions.

Yes, that seems like a real possibility. For example, simplifying a little, when an action is picked up for processing things shake out as follows:

  • Action is fetched
  • Corresponding record is updated to reflect that it is now in progress
  • Action executes
  • Status is updated to 'complete' (or 'failed', etc) as appropriate
  • If it was a recurring action, the next action is now scheduled

So there is a gap between those last two points (the final status update, and the scheduling of the next individual action) and if, concurrently, a call to as_next_scheduled_action() is made then I imagine it will return false, leading to the creation of a surplus action.

Probably, we need to update our docs at the very least to provide a more robust way of handling this. Possibly, we also need some changes in Action Scheduler itself to handle this sort of scenario more gracefully.

@barryhughes barryhughes added the needs discussion Issue that either needs discussion or pending decision (not for support requests). label Jun 15, 2021
@ovidiul
Copy link
Contributor

ovidiul commented Oct 27, 2021

I was able to duplicate this issue as well, with an aside, I've created a batch script that I was running concurrently(4 concurrent processes) to maybe try and simulate race conditions as:

action_scheduler_loop.sh

while true; do wp action-scheduler run; done

Screenshot 2021-10-27 at 09 18 04

In my screenshots, the duplicate appeared at 18:01:21.

I've used this code for every minute(60 seconds) schedule:

/**
 * Schedule an action with the hook 'eg_midnight_log' to run at midnight each day
 * so that our callback is run then.
 */
function eg_schedule_midnight_log() {
	if ( false === as_has_scheduled_action( 'eg_each_minute_log' ) ) {
		as_schedule_recurring_action( strtotime( 'now' ), MINUTE_IN_SECONDS, 'eg_each_minute_log' );
	}
}
add_action( 'init', 'eg_schedule_midnight_log' );

/**
 * A callback to run when the 'eg_each_minute_log' scheduled action is run.
 */
function eg_log_action_data() {
	error_log( 'Date is: ' . date( 'Y-m-d' ) );
}
add_action( 'eg_each_minute_log', 'eg_log_action_data' );

Screenshot 2021-10-27 at 09 02 17

Screenshot 2021-10-27 at 09 08 23

Screenshot 2021-10-27 at 09 02 00

Looking at this code line, that seems the one responsible for creating the duplicate, would it be worth changing the scheduled action store status to failed/complete only after the call to schedule the next event was sent?

@barryhughes
Copy link
Member

Hi!

Would it be worth changing the scheduled action store status to failed/complete only after the call to schedule the next event was sent?

Possibly. I think that would reduce the potential for this problem to occur, but I also suspect the 'core' race condition is the one that arises directly from the snippet found here.

It's pretty aggressive in that it fires every request during init and, especially on a busy site, that could lead to the duplicates being created even if we did update the process_action() method in the way you outlined.

Altering our recommended approach in the docs may therefore be the safer and better approach (short of a larger change that revises how recurring actions work and are stored).

@lsinger lsinger added type: bug The issue is a confirmed bug. priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. and removed needs discussion Issue that either needs discussion or pending decision (not for support requests). labels Sep 29, 2023
@eddr
Copy link

eddr commented Nov 2, 2023

Hello there

Happens to me as well in the same manner
Any news? Can I somehow help with that?

@barryhughes
Copy link
Member

@eddr,

We have a further fix in the works, though efforts have been paused for some time while we prioritized other work. That said, in case you were not already aware of this, it has been possible to declare actions as 'unique' since our 3.6.0 release which may help to reduce instances of recurring event duplication. For instance, you can now replace this:

if ( ! as_has_scheduled_action( 'foo' ) ) {
	as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo' );
}

With:

as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo', [], '', true );

Or, using some PHP 8.x syntax:

as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo', unique: true );

@eddr
Copy link

eddr commented Nov 10, 2023

Thanks barryhughes!
I'm pretty aure i tried the 'unique' parameter, but I test again to make sure and let you know what are the results

@barryhughes
Copy link
Member

Yep, it's not a complete solution: it (hopefully) solves one part of the problem, but there is still a race condition in the logic that schedules the replacement action (which is what the PR I linked to tries to solve).

@eddr
Copy link

eddr commented Nov 12, 2023

I will be happy to test it, if that's of any help

I might be utilizing an unorthodox code, but the 'unique' parameter doesn't help in my case:

  • Running a specific action via \ActionScheduler::runner()->process_action via the 'init' hook. Not sure it's the right way
  • The reason is wanting to make sure it runs every 15 seconds, no matter if the WP cron is scheduled via Linux CRON ( and then probably way less then 15 seconds ) or in the standard way.

I actually think to automatically unhook the actions every now and then via CRON to handle this specific issue.

@barryhughes
Copy link
Member

Thanks for the offer! We may reach out once we're ready for some help.

I suspect you are seeing the problem more frequently, because you're processing things more frequently. We'll try to update the PR I referenced earlier and move this task to completion, but I can't really offer a timeline as we have quite a lot of work on our hands at present.

@eddr
Copy link

eddr commented Dec 12, 2023

For the meanwhile, went for a custom mechanism for my specific needs. Maybe it will be of interest:

  • Used MySQL "SELECT ... FOR UPDATE NOWAIT" to get a lock on a custom _option row and prevent simultaneous runs. I can describe in more details if needed
  • Seems to work well and also not requiring that much SQL queries compared to the usual 100s of queries done anyway. Locks are not waited for. Not sure what is the performance hit for now, but doesn't seem to be a taxing operation in my configuration

@Nemi5150
Copy link
Author

@eddr,

We have a further fix in the works, though efforts have been paused for some time while we prioritized other work. That said, in case you were not already aware of this, it has been possible to declare actions as 'unique' since our 3.6.0 release which may help to reduce instances of recurring event duplication. For instance, you can now replace this:

if ( ! as_has_scheduled_action( 'foo' ) ) {
	as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo' );
}

With:

as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo', [], '', true );

Or, using some PHP 8.x syntax:

as_schedule_recurring_action( time(), HOUR_IN_SECONDS, 'foo', unique: true );

For what it is worth, I recently implemented this and it seems to have solved my problem of duplicate entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants