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

Add as_enqueue_async_action() and async action APIs #322

Merged
merged 3 commits into from
Jul 8, 2019

Conversation

thenbrent
Copy link
Contributor

@thenbrent thenbrent commented Jul 3, 2019

The PR introduces a new set of APIs for scheduling actions to run "as soon as possible". This is to achieve the async action type required as part of #319.

Background

Typically, an action is scheduled for a date. In cases where that action needed to run "as soon as possible", that date would have been set to the current time, e.g. time(). For an example, see WC_Action_Queue::add(). The problem with "now" is that there is no way to make sure that action is run sooner than any other action scheduled to run now, including those that may have been scheduled to run at that time months in advance.

In practice, an async action should take priority over a scheduled action, because there is an expectation it will be run very soon after being added to the queue. Whereas a scheduled action, like a subscription payment, may have been scheduled months in advance so if it processes a few minutes late, most of the time there's no harm done as no one is really sitting around expecting it to go through.

Notes on Approach

This patch makes it acceptable to save an action with a NULL schedule. This mapps its MySQL datetime string to 0000-00-00 00:00:00.

The advantage of this approach is that it is fully backward compatible. Existing queries to claim actions claim them by date, meaning actions with the 0000-00-00 00:00:00 scheduled time will always be claimed prior to actions scheduled for a specific date. This makes sure that any async action is given priority in queue processing. This has the added advantage of making sure async actions can be claimed by both the existing WP Cron and WP CLI runners, as well as a new async request runner.

As this makes async APIs backward compatible with both existing datastores and the new custom table data stores coming in #259, and it has none of the issues mentioned in #319 that would stem from using other backward comptaible APIs, like the using the action group, I've chosen this approach.

The downside is that 0000-00-00 00:00:00 is a bit of a hack and will allow for potentially critical bugs in 3rd party code, as seen by the fact that I had to remove existing validation against NULL dates for it to be possible. 3rd parties can now accidentally schedule actions with a NULL date, instead of receiving an exception to alert them to errors.

@thenbrent thenbrent force-pushed the issue_319_async_action_apis branch from d4610bf to c5876f2 Compare July 3, 2019 05:00
@thenbrent thenbrent mentioned this pull request Jul 3, 2019
1 task
@thenbrent thenbrent added this to the 3.0.0 milestone Jul 3, 2019
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thenbrent this works great but as_next_scheduled_action() doesn't find the async actions. We need that to prevent flooding the queue with actions.

classes/ActionScheduler_ActionFactory.php Show resolved Hide resolved
@thenbrent thenbrent force-pushed the issue_319_async_action_apis branch from c5876f2 to 3f0ed0c Compare July 5, 2019 03:14
@thenbrent
Copy link
Contributor Author

thenbrent commented Jul 5, 2019

but as_next_scheduled_action() doesn't find the async actions.

Ah good catch @rrennick! What do you think is the most sane return value for as_next_scheduled_action() for an async action?

I see the following options:

  1. 0 to represent the absence of a schedule and/or that its scheduled for "the beginning of time" (which is the Unix Epoch in PHP's world). This is risky though, because it will be confused with false via weak comparison operators, so it's likely to create bugs with existing 3rd party code.
  2. 1, to represent a truthy value in int that is still near enough to the beginning of time to communicate the above messages.
  3. time() to indicate that the next time any async action should be run is "now" (which is always true for any pending async action because they should be run ASAP). This could cause issues though, if 3rd party code is checking against something like as_next_scheduled_action() < time(), or for that matter, as_next_scheduled_action() > time().
  4. something else?

It seems like 2 is the best worst option to me.

@thenbrent
Copy link
Contributor Author

Another couple of options:

  1. return NULL, to indicate there is a scheduled action (i.e. the return value is not false), but it doesn't have a scheduled timestamp. This return value has the same issue as 0.
  2. return boolean true, to indicate there is a scheduled action, but it doesn't have a scheduled timestamp. This seems semantically correct, backward compatible and relatively robust. It will fail against any checks for as_next_scheduled_action() > time() though. I'm not sure if there is anyway around that though.

@thenbrent thenbrent force-pushed the issue_319_async_action_apis branch from 8a08d4a to 77d2c09 Compare July 5, 2019 04:02
@thenbrent
Copy link
Contributor Author

but as_next_scheduled_action() doesn't find the async actions.

I've pushed up SHA: 77d2c09 to fix this. It returns true for pending async actions. It continues to return false if there are only cancelled, completed, failed or in-progress async actions with a matching hook.

Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thenbrent Looks & works great. Can we change the target branch to version_3_0_0 ?

thenbrent added 3 commits July 8, 2019 11:35
For scheduling actions that will always be run as soon as possible,
rather than a date in the future, or even now, but only after other
scheduled actions with a date in the past.

This commit makes it acceptable to save an action with a NULL schedule
by mapping its MySQL datetime string to 0000-00-00 00:00:00. This
makes sure that any "async" action is given priority in queue processing.
as_next_scheduled_action() will only ever check for an action with
the pending status, which makes sense. This wasn't clear though,
becuase the pending status filter was only applied via the default
find_action() param value in ActionScheduler_wpPostStore.

To improve compatibility with other data stores, and avoid developers
having to check the stack, it makes sense to explicitly declare that
status filter within the params defined in as_next_scheduled_action().
The new async psuedo action type uses the NullSchedule to map to a
0000-00-00 00:00:00 date in MySQL and ensure it's always run as
soon as possible. However, that meant as_next_scheduled_action()
was returning boolean false for the next scheduled timestamp on
these, becuase their schedule()->next() value was NULL.

We need to return something other than false, because there are
pending actions.

There were a number of options for this return value:

1. int 0 to represent the absence of a schedule and/or that its
   scheduled for "the beginning of time" (which is the Unix Epoch in
   PHP's world). This is risky though, because it will be confused with
   false via weak comparison operators, so it's likely to create bugs
   with existing 3rd party code.
2. int 1, to represent a truthy value in int that is still near enough
    to the beginning of time to communicate the above messages.
3. time() to indicate that the next time any async action should be run
   is "now" (which is always true for any pending async action because
   they should be run ASAP). This could cause issues though, if 3rd party
   code is checking against as_next_scheduled_action() < time(), or for
   that matter, as_next_scheduled_action() > time().
4. NULL, to indicate there is a scheduled action (i.e. the return value
   is not false), but it doesn't have a scheduled timestamp. This return
   value has the same issue as 0.
5. return boolean true, to indicate there is a scheduled action, but it
   doesn't have a scheduled timestamp. This seems semantically correct,
   backward compatible and relatively robust. It will fail against any
   checks for as_next_scheduled_action() > time() though. I'm not sure
   if there is anyway around that though.
@thenbrent thenbrent force-pushed the issue_319_async_action_apis branch from 77d2c09 to cacacea Compare July 8, 2019 01:35
@thenbrent thenbrent changed the base branch from master to version_3_0_0 July 8, 2019 01:36
@thenbrent
Copy link
Contributor Author

thenbrent commented Jul 8, 2019

Can we change the target branch to version_3_0_0 ?

Yes of course! I've cherry-picked the commits from this branch to a new branch based on version_3_0_0, force pushed and changed the base branch on this PR to version_3_0_0.

I originally wrote this patch using add/custom-tables as the base (on SHA: 5af3d17), so it should work fine once those branch in merged too (depending on changes from that SHA to merge).

@rrennick rrennick merged commit 5649d06 into version_3_0_0 Jul 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the issue_319_async_action_apis branch July 8, 2019 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants