Move locking to PHP (and remove scheduler_cron.sh)

[fbrnc]

(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]

+1

[steverobbins]

Guess that's the end of #143

[fbrnc]

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]

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]

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 [email protected] 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
#190 (comment)
.

Robbie Averill
LA PETITE MANOUCHE - NZ Gypsy Jazz Guitar Duo
Phone: +64 27 321 4758
[email protected]
www.lapetitemanouche.com

[ihor-sviziev]

FYI we have lock realization in Mage_Index_Model_Lock

[LeeSaferite]

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

[fbrnc]

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

• We should handle locking in PHP (and might use the mechanism Magento already provides with Mage_Index_Model_Lock)
• We should handle locking only (?) on a schedule level and don't care about multiple php processes running at the same time (since the might finish early once they detect they have nothing to do since jobs are locked).

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]

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]

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]

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]

Also see #176