[Bug 23571] New: fines.pl may block server
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Bug ID: 23571 Summary: fines.pl may block server Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: hagud@orex.es QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Under certain circunstances fines.pl may block a server. 1. if syspref calculate on return 2. If koha site has a big number of overdue items 3. there are puntual high number of checkin under this circunstances koha launches several fines.pl,each for every koha checkin, that overload cpu and make koha unresponsive This actions should be desiderable 1. fines.pl on return just calculate the user account, not all users Another option is possible, block fines.pl if there is another process running.. but perhaps option 1 is faster? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |tomascohen@gmail.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|tomascohen@gmail.com |agustinmoyano@theke.io Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |amit.gupta@informaticsgloba | |l.com, | |amitddng135@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #1 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 96055 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96055&action=edit Bug 23571: [DO NOT PUSH] crash condition builder This patch adds t/db_dependent/bug_23571_crash_condition_builder.t file This file is not intended to be part of Koha, but it's used to emulate crash conditions. It will create 5000 users, 5000 biblios with 2 items each, and each item issued to a user. It really takes a lot of time, so please be zen about it ;) To use it execute prove t/db_dependent/bug_23571_crash_condition_builder.t Sponsored-by: Orex Digital -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #2 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 96056 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96056&action=edit Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #3 from Hugo Agud <hagud@orex.es> --- I have installed the patch , and from sudo koha-shell kohadev I have launched two terminal session but there is no error message at second terminal, perhaps I have done something wrong SESSION 1 kohadev-koha@kohadevbox:/tmp$ perl /home/vagrant/kohaclone/misc/cronjobs/fines.pl -v -l -o Use of uninitialized value $lockfile in concatenation (.) or string at /home/vagrant/kohaclone/misc/cronjobs/fines.pl line 93. WARNING: Could not create lock file : No such file or directory Verify if directories /var/lock/koha/koha_kohadev, /var/lock/koha_fines_koha_kohadev or /tmp/koha_fines_koha_kohadev exist and check file permissions writing to /var/lib/koha/kohadev/tmp/koha_kohadev_2020-01-07.log Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. Use of uninitialized value in numeric eq (==) at /home/vagrant/kohaclone/C4/Overdues.pm line 266. Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. SESSION 2 kohadev-koha@kohadevbox:/home/vagrant$ perl /home/vagrant/kohaclone/misc/cronjobs/fines.pl -v -l -o Use of uninitialized value $lockfile in concatenation (.) or string at /home/vagrant/kohaclone/misc/cronjobs/fines.pl line 93. WARNING: Could not create lock file : No such file or directory Verify if directories /var/lock/koha/koha_kohadev, /var/lock/koha_fines_koha_kohadev or /tmp/koha_fines_koha_kohadev exist and check file permissions writing to /var/lib/koha/kohadev/tmp/koha_kohadev_2020-01-07.log Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. Use of uninitialized value in numeric eq (==) at /home/vagrant/kohaclone/C4/Overdues.pm line 266 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16528 CC| |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Circulation |Command-line Utilities CC| |martin.renvoize@ptfs-europe | |.com, robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=24651 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96055|0 |1 is obsolete| | --- Comment #4 from Hugo Agud <hagud@orex.es> --- Created attachment 98898 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98898&action=edit Bug 23571: [DO NOT PUSH] crash condition builder This patch adds t/db_dependent/bug_23571_crash_condition_builder.t file This file is not intended to be part of Koha, but it's used to emulate crash conditions. It will create 5000 users, 5000 biblios with 2 items each, and each item issued to a user. It really takes a lot of time, so please be zen about it ;) To use it execute prove t/db_dependent/bug_23571_crash_condition_builder.t Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96056|0 |1 is obsolete| | --- Comment #5 from Hugo Agud <hagud@orex.es> --- Created attachment 98899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98899&action=edit Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98899|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99263 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99263&action=edit Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99263|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99264 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99264&action=edit Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99265 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99265&action=edit Bug 23571: (follow-up) Only look for overdues after obtaining the lock No point in performing the overdues fetch before obtaining the lock and knowing we can do something with them. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 99264 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99264 Bug 23571: Add flock to fines.pl Review of attachment 99264: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=23571&attachment=99264) ----------------------------------------------------------------- So, this generally looks pretty good to me.. However... I'd love to see the locking mechanisms factored out into the Koha::Script base class so we could share the common code between command-line scripts and make adding flocking elsewhere a simple pattern to follow. Would you be willing to have a go at that Agustin? ::: misc/cronjobs/fines.pl @@ +82,5 @@
cronlogaction();
+my ($lockfile, $LockFH); +foreach ( + '/var/lock/koha',
How did you come up with these locations? I note in the equivalent in rebuild_zebra.pl we use a config variable, followed by a fallback order... I actually like the idea of using /var/lock/koha but I'm not sure I understand where if came from? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- A couple of things: 1. test plan says "the second one fails", but it actually prints. I would die instead. 2. That will make you avoid the indentation level you add with this patch. The algo would be: - try to get a lock - if lock already exists then exit/die with "cannot get lock" (or something more meaningful: "previous run is still alive, cannot start") - continue the normal process of the script -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #10)
A couple of things: 1. test plan says "the second one fails", but it actually prints. I would die instead.
s/would/should! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Also, do you know that you can lock a portion of the file? I have no idea why your code is that complicated, I think you should investigate this code: use Modern::Perl; use Fcntl qw(:flock); sub is_locked { unless ( flock(DATA, LOCK_EX|LOCK_NB) ) { return 1; } return 0; } die "is locked" if is_locked(); say "is not locked! sleeping 5 sec"; sleep(5); __DATA__ USED BY LOCK - DO NOT REMOVE -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Failing QA. Agustin please investigate comment 12 and tell us if there is a possibility to simplify your code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|fines.pl may block server |Prevent concurrent | |execution of fines.pl Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 102772 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102772&action=edit [ALTERNATIVE] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |25109 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25109 [Bug 25109] Add execution locking to Koha::Script -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #9)
Comment on attachment 99264 [details] [review] Bug 23571: Add flock to fines.pl
Review of attachment 99264 [details] [review]: -----------------------------------------------------------------
So, this generally looks pretty good to me.. However... I'd love to see the locking mechanisms factored out into the Koha::Script base class so we could share the common code between command-line scripts and make adding flocking elsewhere a simple pattern to follow.
Would you be willing to have a go at that Agustin?
::: misc/cronjobs/fines.pl @@ +82,5 @@
cronlogaction();
+my ($lockfile, $LockFH); +foreach ( + '/var/lock/koha',
How did you come up with these locations?
I note in the equivalent in rebuild_zebra.pl we use a config variable, followed by a fallback order... I actually like the idea of using /var/lock/koha but I'm not sure I understand where if came from?
Take a look at 25109, in which I introduce a centralized place to calculate this path. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #10)
A couple of things: 1. test plan says "the second one fails", but it actually prints. I would die instead. 2. That will make you avoid the indentation level you add with this patch. The algo would be: - try to get a lock - if lock already exists then exit/die with "cannot get lock" (or something more meaningful: "previous run is still alive, cannot start") - continue the normal process of the script
Solved in my alternative patch. I chose to keep the output silent, unless --verbose is passed. This is the right behaviour for cronjobs, specially if we have the execution logged in the action_logs. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #12)
Also, do you know that you can lock a portion of the file?
I loved the trick when I read about it while implementing 25109. But as we support multiple instances running the same codebase, this won't work. We need a 'namespace' (per instance) and the only way I found to do it was in a lock file. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #13)
Failing QA. Agustin please investigate comment 12 and tell us if there is a possibility to simplify your code.
Done :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Awesome guys, this is really great to see. 😀. I'll put it high up my list to start playing with these bugs with a view to SO/QA after the Easter break. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Can't locate object method "new" via package "Koha::Scrip" (perhaps you forgot to load "Koha::Scrip"?) at misc/cronjobs/fines.pl line 80. typo. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98898|0 |1 is obsolete| | Attachment #99264|0 |1 is obsolete| | Attachment #99265|0 |1 is obsolete| | Attachment #102772|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102804 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102804&action=edit Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #21)
Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block)
"pach", that's quite ironical :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart@bugs.koha-c |y.org |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|Small patch |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102804|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 102821 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102821&action=edit Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|agustinmoyano@theke.io |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102821|0 |1 is obsolete| | --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 102844 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102844&action=edit Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why no "\n" in the print statement, is that expected? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #25)
Why no "\n" in the print statement, is that expected?
I think I only missed it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 102888 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102888&action=edit Bug 23571: (QA follow-up) Add missing newline in warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102844|0 |1 is obsolete| | Attachment #102888|0 |1 is obsolete| | --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102910 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102910&action=edit Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #29 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102911 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102911&action=edit Bug 23571: (QA follow-up) Add missing newline in warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102912 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102912&action=edit bug 23571: (QA follow-up) Remove perlcritic warning Bareword file handle opened at line 138, column 5. See pages 202,204 of PBP. (Severity: 5) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com --- Comment #32 from Joy Nelson <joy@bywatersolutions.com> --- does not apply cleanly (Koha/Script.pm) to 19.11.x branch. please rebase if needed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Prevent concurrent |Add measures to prevent |execution of fines.pl |concurrent execution of | |fines.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Bug 23571 depends on bug 25109, which changed state. Bug 25109 Summary: Add execution locking to Koha::Script https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25109 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 Blou <philippe.blouin@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philippe.blouin@inlibro.com --- Comment #33 from Blou <philippe.blouin@inlibro.com> ---
Solved in my alternative patch. I chose to keep the output silent, unless --verbose is passed. This is the right behaviour for cronjobs, specially if we have the execution logged in the action_logs.
I'd like to understand how it is "the right behavior" for a process to not output a message when a fatal error occurs ? I've seen the option -q added for quietness, but never to require -v to print fatal errors. We ran the script manually and got nothing telling us it failed. The action logs is NOT a valid alternative for meaningful (and efficient) error messages. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23571 --- Comment #34 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Blou from comment #33)
Solved in my alternative patch. I chose to keep the output silent, unless --verbose is passed. This is the right behaviour for cronjobs, specially if we have the execution logged in the action_logs.
I'd like to understand how it is "the right behavior" for a process to not output a message when a fatal error occurs ? I've seen the option -q added for quietness, but never to require -v to print fatal errors.
We ran the script manually and got nothing telling us it failed. The action logs is NOT a valid alternative for meaningful (and efficient) error messages.
As the script is (by default) only called on a daily basis, it may be relevant to send to STDERR the lock error (even without --verbose). Tomas, what do you think? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org