Review and revise exception handling within Action Scheduler #1059
Comments
|
Hi @alewolf, Are you quite sure the problem is specifically tied to 3.7.4? This was a fairly minimal set of changes. What version did you switch to, and have you found that that has resolved the problems (at least in cases where WooCommerce, or other plugins shipping 3.7.4, are not present)?
We could probably finesse some of these, thank you for flagging. For what it's worth, errors of this type may point to a race condition between two queue runners, or between Action Scheduler's own queue runners and some custom code.
I'm sure this can be annoying, but things are not always quite as clear cut as you are suggesting. We do support Action Scheduler, here and to a lesser extent in the WordPress community forums, but Action Scheduler is a conduit for processing work: errors surface through it, but not always because of it. With your experience, you may understand when and how to make the distinction, but I'm not sure it's realistic to expect large support teams to make the right call 100% of the time. Even in this case, it's not completely clear that the problem has its origin in Action Scheduler.
These exceptions are not new, and as above I'm not convinced they are tied to 3.7.4 specifically. Definitely let me know if I'm missing something obvious, here.
Though I can take some guesses as to what you'd like to see here, I'd much rather get your informed opinion. When an unexpected condition is met, should it: fail silently, log the errors somewhere else, carry on regardless? When should we let site operators know of problems, and when should we suppress them? |
|
Hi @barryhughes
I upgraded from I can't say if it is bound to
As explained in my earlier post, I've downgraded to one version below WooCommerce. As expected it will now hit another install. Now Rank Math will get those support requests:
|
This doesn't feel like the right direction, to me. I'm curious, though: in the case of your plugin it seems like WooCommerce is a requirement. What prevents you from using the WC Queue, or from using WooCommerce's copy of Action Scheduler directly (vs bundling your own)? |
Good question. Technically the Pixel Manager can also be used on installs where WooCommerce is not running (because some of our users don't want to use more than one tagging solution). Until now I haven't thought about loading the Action Scheduler conditionally, depending if WooCommerce is active or not. However, I fear that it might not be possible to implement conditional loading. The Action Scheduler needs to be required very early in the loading process. If I understand correctly this is to make sure that the Action Scheduler can check if only one, and only the most recent version is loaded, in case more than one plugin tries to load it. However, it is not possible to check if WooCommerce is active so early. Let me know if you think this is possible. If so, I will change the logic and only load it conditionally and let WooCommerce take over. |
Gotcha, I misunderstood in that case.
It probably is possible, but given the context I suspect the potential risk and complexity may not be worth the gain. |
vedanshujain
added
the
priority: normal
This was reported on the community forums. Noting it here because, similar to the work we did in #1060, we can probably add additional context to make things clearer. |
barryhughes
changed the title
3.7.4 regularly throws InvalidArgumentException
|
Updated title and description. We're hoping to look at options here soon. |
|
@barryhughes Would this error be one that you would expect an author of a plugin (bundling AS) to handle? That is to say, if a customer came to WooCommerce support about the above error, would your internal process direct to investigate further, or simply send away to whichever plugin bundled Action Scheduler? Certainly there are errors that are because of Action Scheduler, and not because of the bundling plugin. Perhaps including in the error message direction on who is the best first contact would be helpful. Off the top of my head, this could potentially be determined by checking I think @alewolf has a very valid point, that plugin support teams can potentially get flooded with support requests that are not related to their plugin, and everyone involved should be do all they can to keep that from happening, especially Action Scheduler, being the common technology. If it's not handled well, then plugin authors will do exactly as @alewolf has done, and use older versions in an attempt to avoid the support burden. Not exactly a winning approach to encourage developers to use the latest version. |
|
@alewolf In terms of conditionally loading, the introduction of Plugin Dependencies in WP 6.5 may give you a more viable approach: /**
* Plugin name: Pixel Manager for WooCommerce
* Requires Plugins: action-scheduler
*/
...
if ( version_compare( $wp_version, '6.5' ) < 0) {
// require bundled AS
}This does add an additional step for site admins, but is an option to use the latest version while also alleviating support tickets. As a site admin, I always prefer installing AS explicitly anyways; maybe there are more like me. |
Definitely. Others are not, and yet others occupy a grey area somewhere in-between (and relate to the volume of work being processed, alongside the available resources).
Yep, so do we, hence the latest update. |
Oh sorry, I thought this ticket was generalized to changing exception handling and messages, not specific to alleviating false support tickets. |
|
Yes—that's the primary technical focus. The support aspect is one of the motivations for doing this, and something we'll look to alleviate through those changes. |
↑ This is already caught, by |
↑ This is already caught, by (Edit: 2 exceptions are thrown from this method, but both are covered.) |
↑ This is already caught, by |
↑ Both of the above are part of the current nested try/catch used for error handling. The exceptions are caught in the very same structure, in the same method (but we can probably simplify this now that we don't need to support PHP 5.6) 🟠 Let's review the nested try/catch. Low priority. |
↑ Unclear who/what we're guarding against here. I'd think if a bulk action lacks a valid callback, we should log that but simply omit it from the returned array of possible actions. Needs attention, low priority (I don't recall seeing reports about this specific exception) 🟠 |
↑ This code should only run in the context of WP CLI. The exception seems justified, no follow-up needed at present 🟢 |
↑ This code should only run in the context of WP CLI. The exception seems justified, no follow-up needed at present 🟢 |
↑ For all of the above, the exception is thrown from within a private method but we trigger this through various paths. In some cases, we catch the exception then log an error. In others, we do not and execution probably halts. Needs review 🔴 |
↑ These are all within |
There are quite a few occasions when Action Scheduler experiences an unexpected condition, and (mostly for safety) it stops. It communicates about this using exceptions, but many of these exceptions:
In most cases, this is 'fine' in the sense that the queue is processed in requests that are independent of user-facing requests. However, they could impact other scheduled events when running in the context of WP Cron.
Additionally, there is the problem noted below which is that only one version of Action Scheduler can be active at any one time. That is, only one parent plugin 'wins' this particular race, and if things go wrong—irrespective of the underlying reason—the backtraces often lead users back to that plugin's support team.
Let's review and revise when we throw exceptions, and when we should add handling for them.
action_idof0to indicate they are not related to a specific action). If we do this, we would also want to add:Original report follows...
I bundle the ActionScheduler version
3.7.4in the Pixel Manager for WooCommerce.As of recently we started to receive critical error reports like this:
3.7.4of the ActionScheduler. It seems that the version shipped by the Pixel Manager is loaded first (probably because of the alphabetical order). The error path shows the Pixel Manager and we get all support requests.19061390exists and has just failed to execute.3.7.4will have the same issue. Unwarranted support requests because the ActionScheduler doesn't handle failed actions gracefully and logs them in the fatal-errors log.The text was updated successfully, but these errors were encountered: