action-scheduler icon indicating copy to clipboard operation
action-scheduler copied to clipboard

Allow 3rd-party code to enable asnyc runner requests outside of the admin context

Open danielbitzer opened this issue 5 years ago • 5 comments

Adds a filter (action_scheduler_allow_non_admin_async_runner_request) that can be used by 3rd-party code to allow an async runner request to be dispatched outside the admin context.

I deliberately prevented the filter from disabling async runner requests in the admin area to prevent this from accidentally happening if the filter is misused.

@rrennick @thenbrent as discussed we agreed this would be the best solution to enable AutomateWoo to completely switch to use ActionScheduler.

I've added this to the 3.2.0 milestone but would love to get this in as soon as possible.

cc @woocommerce/ventures

danielbitzer avatar Oct 16 '20 06:10 danielbitzer

@danielbitzer Thanks for submitting the PR. This looks great. I updated the base to version_3_2_0. I'm going to leave the official approval for @Konamiman or @vedanshujain .

rrennick avatar Oct 16 '20 12:10 rrennick

It may be out of scope for this, but I think it fits. Could the action_scheduler_allow_async_request_runner filter be pulled out and run before all this? At the moment the following is all run before the filter is called: the locks are checked, a query for concurrency batches currently running, and a query to see if due actions are available. Post this PR, there will be another potential thing running unnecessarily.

But the only purpose of this filter is to disable the async request runner completely. So if we're doing that, could we check the filter much earlier and avoid all the above? Perhaps at the start of maybe_dispatch_async_request() after the is_admin() check.

WPprodigy avatar Jun 24 '21 03:06 WPprodigy

Hi @danielbitzer and @rrennick, sorry for the huge delay, is this still relevant? If so it needs to be de-conflicted, and I guess the @since tag will need to be updated too.

Konamiman avatar Aug 19 '22 09:08 Konamiman

Hi @danielbitzer and @rrennick, sorry for the huge delay, is this still relevant?

@jconroy do you know if this is still relevant?

rrennick avatar Aug 22 '22 13:08 rrennick

Since the implementation was delayed, a workaround was introduced directly in AutomateWoo to add it's own async runner The workaround has been running for a while now on various sites, but it has come up a few times in various issues.

Looking at the code change in this PR I'd be hesitant about the following comment:

		 * Performance note: This filter is evaluated at 'shutdown' on every non-admin request.
		 * Avoid attaching database queries or intensive work to this filter.

Since the filter is either enabled or disabled without passing any context. Plugin X can enable it and plugin Y can schedule a task with "intensive work". Personally I think we should look more into the direction of enabling that extra queue runner only for a specific group/priority of tasks. Which could prevent any "heavier" tasks to be run at shutdown on a frontend request. Although that's outside the scope of this PR.

mikkamp avatar Aug 22 '22 14:08 mikkamp

I tend to agree with @mikkamp 's comment. This has the potential to cause performance problems and there are probably better solutions to the problem out there, so alternative PRs are welcome. We'll close this one as it seems like the solution's tradeoffs are not worth it at this time.

peterfabian avatar Sep 30 '22 08:09 peterfabian