AOEpeople / Aoe_Scheduler

Cron Scheduler Module for Magento
http://fbrnc.net/blog/2011/03/magento-cron-schedule
GNU General Public License v3.0
373 stars 202 forks source link

Move locking to PHP (and remove scheduler_cron.sh) #190

Open fbrnc opened 9 years ago

fbrnc commented 9 years ago

(Extracted from one of my comments in #176)

Since the solely purpose of this bash script is to handle locking I'm wondering why we're not handling this inside PHP since we're calling our own shell/scheduler.php anyway at this point.

We'd eliminate some complexity and wouldn't have to deal with an extra layer. If we decide do to this we should keep the scheduler_cron.sh around for a while to be backwards compatible while updating the instructions in the backend and the documentation.

robbieaverill commented 9 years ago

+1

steverobbins commented 9 years ago

Guess that's the end of https://github.com/AOEpeople/Aoe_Scheduler/pull/143

fbrnc commented 9 years ago

Actually, no it's not. I have actually been thinking about that while creating this issue. Here are my thoughts:

I want to eliminate the extra bash layer for most users, but that doesn't mean that we couldn't have a wrapper script for something like the daemon mode. I continue to like the idea of a daemon, I just haven't had the chance to have a closer look at @steverobbins' PR (#134 / #143) look at everything we discussed and decide if I want to merge it like this or not :)

steverobbins commented 9 years ago

It's not unheard of to daemonize with PHP (PHPCI comes to mind).

Thought I'm not sure it will behave the same, and using exec() still sort of counts as a bash layer.

robbieaverill commented 9 years ago

IMO a bash daemon would be better. Having used PHP for a daemon in a previous project it runs into issues under high load.

On Monday, 9 November 2015, Steve Robbins notifications@github.com wrote:

It's not unheard of to daemonize with PHP (PHPCI https://github.com/Block8/PHPCI/blob/master/daemonise comes to mind).

Thought I'm not sure it will behave the same, and using exec() still sort of counts as a bash layer.

— Reply to this email directly or view it on GitHub https://github.com/AOEpeople/Aoe_Scheduler/issues/190#issuecomment-154934457 .

Robbie Averill LA PETITE MANOUCHE - NZ Gypsy Jazz Guitar Duo Phone: +64 27 321 4758 contact@lapetitemanouche.com www.lapetitemanouche.com

ihor-sviziev commented 9 years ago

FYI we have lock realization in Mage_Index_Model_Lock

LeeSaferite commented 9 years ago

I have a scheduler branch that does per scheduled job locking as well.

fbrnc commented 9 years ago

Ok, so it looks like using a bash script to prevent parallel execution wasn't a good idea to begin with. To summarize:

Also, Magento's default cron already does some per-scheduler-locking: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/core/Mage/Cron/Model/Schedule.php#L216-L219 @LeeSaferite: How's you approach different from that?

fbrnc commented 9 years ago

Ok, I think I confused locking a specific schedule with locking a specific job (type). So the built-in mechanism only makes sure that one specific schedule won't be processed by to different processes, but doesn't protected from two schedules with the same job-code to be processed.

andrey-legayev commented 8 years ago

I've just discovered locking issue in scheduler_cron.sh in version 1.2.2: Lock folder created by sh script in /tmp/ wasn't deleted on server reboot. I see "trap" in code, but somehow it didn't help. It caused ~24 hours of signle cron jobs group outage - nothing serious, but it wasn't expected.

It would be good to switch from file locking based on file/folder existence to flock() which is releasing exclusive lock once process exits. This idea is already implemented in Mage_Index_Model_Lock in PHP, but I think it may be also implemented in bash as temporary solution inside scheduler_cron.sh. Sounds good?

fbrnc commented 8 years ago

Hi @andrey-legayev,

I like the idea of moving the locking logic inside PHP and even better if there's already a concept in the Magento core that we can use (if that makes sense). I'll gladly review your PR in case you want to look into this. I suggest keeping scheduler_cron.sh even if it doesn't do anything except executing php scheduler.php for backwards compatibility reasons.

Cheers, Fabrizio

fbrnc commented 8 years ago

Also see #176