Neither process_message_queue.pl nor C4::Letters::SendQueuedMessages have any method of ensuring that duplicate emails are not sent if two or more instances of process_message_queue.pl are running concurrently. This seems to have caused issues on a server which was re-running cron jobs because the system clock was re-set by NTP, but it could also cause issues if process_message_queue.pl took a very long time to process.
should set a flock like we did for zebra.
I believe that this may also cause fragmenting of CHECKOUT, RENEWAL and CHECKIN messages. These are digest messages, which should accumulate detail about their respective circ action in message_queue.metadata until process_message_queue.pl runs. We have seen evidence of multiple RENEWAL messages going out in very short order... it would be normal for process_message_queue.pl to be running while items are being checked in our out, causing two messages to be sent, but we have had evidence of 3 messages sent at the same time, which isn't possible unless the message_queue is being processed by two separate instances of process_message_queue.pl
Barton, take a look at: Bug 15854 - Race condition for sending renewal/check-in notices This is probably responsible for the multiple messages.
Created attachment 78205 [details] [review] Bug 15529 - Using flock to lock new messagequeue.lock lockfile whilst process_message_queue.pl is being executed Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch 3. Repeast step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues 5. At the end of the execution of the first script a 15 second pause is run Sponsored-By: Parliament Library
I have attached a preliminary patch which will be refined further in the near future.
Created attachment 78210 [details] [review] Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl The script loops through every enabled instance. It checks if the messagequeue.lock file is locked in the instance directory. If not then the cronjob runs. If the lock file is locked then this is logged and a email is sent out. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch 3. Repeast step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues 5. At the end of the execution of the first script a 15 second pause is run Sponsored-By: Parliament Library
Created attachment 78211 [details] [review] Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl The script loops through every enabled instance. It checks if the messagequeue.lock file is locked in the instance directory. If not then the cronjob runs. If the lock file is locked then this is logged and a email is sent out. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch 3. Repeast step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues Sponsored-By: Parliament Library
Created attachment 78212 [details] [review] Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl The script loops through every enabled instance. It checks if the messagequeue.lock file is locked in the instance directory. If not then the cronjob runs. If the lock file is locked then this is logged and a email is sent out. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch 3. Repeast step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues Sponsored-By: Parliament Library
Comment on attachment 78212 [details] [review] Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl AFAICS, no other cronjob script uses one of the debian/scripts/koha- -scripts ... I wonder if this works for everyone?
Created attachment 78582 [details] [review] Bug 15529 - Locking process_message_queue.pl cron whilst it is running This amended patch is called by koha-common. koha-common runs this process_message_queue.pl cronjob foreach enabled Koha instance on the server. As this cronjob is run for every Koha instance the retrival of all Koha instance names from debian/scripts/koha-scripts in previous obsolete patches on this bug report and the looping through every name to check if the lock file existed is redundant. Instead this patch retrieves the 'logdir' of the Koha instance being iterated over and checks if the messagequeue.lock file in that directory is locked. If the aforementioned file is not locked then the SendQueuedMessages() in C4/Letters.pm is called, otherwise an email is sent (to the manually inputted email server, and to/from addresses) and duplicate execution of the cronjob is prevented. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch and input the email server, to/from email addresses you would like the email to be sent to. Also confirm your Koha instance is enabled to send emails. 3. Repeat step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues 5. At the end of the execution of the first script a 15 second pause is run 6. Notice you have received an email Sponsored-By: New Zealand Parliamentary Library
Created attachment 78918 [details] [review] Bug 15529 - Locking process_message_queue.pl cron whilst it is running This amended patch is called by koha-common. koha-common runs this process_message_queue.pl cronjob foreach enabled Koha instance on the server. As this cronjob is run for every Koha instance the retrival of all Koha instance names from debian/scripts/koha-scripts in previous obsolete patches on this bug report and the looping through every name to check if the lock file existed is redundant. Instead this patch retrieves the 'logdir' of the Koha instance being iterated over and checks if the messagequeue.lock file in that directory is locked. If the aforementioned file is not locked then the SendQueuedMessages() in C4/Letters.pm is called, otherwise an email is sent (to the manually inputted email server, and to/from addresses) and duplicate execution of the cronjob is prevented. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch and input the email server, to/from email addresses you would like the email to be sent to. Also confirm your Koha instance is enabled to send emails. 3. Repeat step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues 5. At the end of the execution of the first script a 15 second pause is run 6. Notice you have received an email Sponsored-By: New Zealand Parliamentary Library
Rebased against master on 16th of sept 2018.
Comment on attachment 78918 [details] [review] Bug 15529 - Locking process_message_queue.pl cron whilst it is running Review of attachment 78918 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/process_message_queue.pl @@ +100,5 @@ > +); > + > +$email->attach( > +Type => 'Text', > + Data => 'process_message_queue.pl cron is already running', If another process is running, why not queue this as a message, rather than add another layer of emailing ugliness?
Created attachment 79649 [details] [review] Bug 15529 - Wrapping email sending up in the verbose flag Thus the cron email telling the administrator or support company that a second execution of the process_message_queue.pl cron whilst it is already running is only sent if it is wanted and verbose is set. Sponsored-By: New Zealand Parliamentary Library
(In response to M. Tompsett in comment 13): >If another process is running, why not queue this as a message, rather than add another layer of emailing ugliness? Thanks for your feedback M. Tompsett. I had a conversation about this with Liz Rea and we came to the conclusion that the benefit of sending it immediately rather than enqueuing the email is because the administrator (or support company) knows straight away and can go and troubleshoot. My latest patch addition wraps the sending of the email up in the verbose flag so the email is only sent if you want it to be, by default it will not be sent.
Then why not trigger the process_messages.pl immediately after queuing? Adding more libraries is still very ugly. my $return = qx{echo "foobar"};
How about this?
Created attachment 79719 [details] [review] Bug 15529: Followup to send without more libraries This uses the letter_code parameter to limit the current duplicate run of the process_message_queue.pl script to just this one message. We don't care about receiving duplicate warning messages as much as we care about sending patrons duplicate messages.
Apply ALL the patches.
Comment on attachment 79719 [details] [review] Bug 15529: Followup to send without more libraries Forgot to git commit changes first. :)
Created attachment 79726 [details] [review] Bug 15529: Followup to send without more libraries This uses the letter_code parameter to limit the current duplicate run of the process_message_queue.pl script to just this one message. We don't care about receiving duplicate warning messages as much as we care about sending patrons duplicate messages.
Hi M. Tompsett I have tested and your patch, in combination with my first two patches, works nicely. I will sign off on your patch, can you please do the same on my two patches. Cheers, Alex
Created attachment 79910 [details] [review] Bug 15529: Followup to send without more libraries This uses the letter_code parameter to limit the current duplicate run of the process_message_queue.pl script to just this one message. We don't care about receiving duplicate warning messages as much as we care about sending patrons duplicate messages. Works as described, passes QA test tool. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 79911 [details] [review] Bug 15529 - Locking process_message_queue.pl cron whilst it is running This amended patch is called by koha-common. koha-common runs this process_message_queue.pl cronjob foreach enabled Koha instance on the server. As this cronjob is run for every Koha instance the retrival of all Koha instance names from debian/scripts/koha-scripts in previous obsolete patches on this bug report and the looping through every name to check if the lock file existed is redundant. Instead this patch retrieves the 'logdir' of the Koha instance being iterated over and checks if the messagequeue.lock file in that directory is locked. If the aforementioned file is not locked then the SendQueuedMessages() in C4/Letters.pm is called, otherwise an email is sent (to the manually inputted email server, and to/from addresses) and duplicate execution of the cronjob is prevented. Test plan: 1. In two terminals open koha-shell and try to run ./process_message_queue.pl and notice you can run the script multiple times simulataneously without a warning/error being displayed 2. Apply patch and input the email server, to/from email addresses you would like the email to be sent to. Also confirm your Koha instance is enabled to send emails. 3. Repeat step 1 and notice that the second of the two scripts to be run will output text saying './process_message_queue.pl is already running. Exiting' In this case the aforementioned text is displayed and the body of the cronjob is not run. 4. The first cronjob script to run, runs without issues 5. At the end of the execution of the first script a 15 second pause is run 6. Notice you have received an email Sponsored-By: New Zealand Parliamentary Library Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 79912 [details] [review] Bug 15529 - Wrapping email sending up in the verbose flag Thus the cron email telling the administrator or support company that a second execution of the process_message_queue.pl cron whilst it is already running is only sent if it is wanted and verbose is set. Sponsored-By: New Zealand Parliamentary Library Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 79913 [details] [review] Bug 15529: Followup to send without more libraries This uses the letter_code parameter to limit the current duplicate run of the process_message_queue.pl script to just this one message. We don't care about receiving duplicate warning messages as much as we care about sending patrons duplicate messages. Works as described, passes QA test tool. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
I signed off Alex's having tested with all three. Alex has signed off mine having tests with all three. Marking as signed off.
I don't like that we're inexplicably adding a lock file to a logging directory.. we could use similar logic to rebuild_zebra.pl which takes either a config line (zebra_lockdir), a standard location (/var/loc/zebra_instance) or failing those two locations it falls back to /tmp/zebra_instance. Otherwise, I'm reasonably happy with this patch with it's followups.
See bug 11078 or the current rebuild_zebra.pl script for inspirations perhaps ;)
Created attachment 80256 [details] [review] Bug 15529: Altered logic for lockfile creation Implemented logic for creating lockfile preventing a lock file being added to a logging directory. This patch adds logic (outlined below) similar to what is in use in rebuild_zebra.pl. The logic is: Create lockfile in either: 1. Location defined in config (cron_lockdir) OR 2. A standard location (/var/koha/cron_<instancename>) OR 3. /tmp/cron_<instancename> Sponsored-By: New Zealand Parliamentary Library
Hi Martin Thanks for testing. I have taken a look at rebuild_zebra.pl and the patches on bug report 11078 and have implemented that logic for where the lock file is created. Can you please take a look and let me know if further changes are required to my followup patch I have just attached. Many thanks, Alex
Comment on attachment 80256 [details] [review] Bug 15529: Altered logic for lockfile creation Review of attachment 80256 [details] [review]: ----------------------------------------------------------------- Thanks for the super quick followup.. some minor issue which I'm happy to take care of as a qa followup. I'm just getting another qa to take a quick look to varify my understanding of the packages scripts then I'll do the followup and pass. ::: misc/cronjobs/process_message_queue.pl @@ +40,4 @@ > my $help = 0; > my $verbose = 0; > my $type = q{}; > +my $use_flock; I don't think this is needed.. unless we want to allow optionally falling back to not using lock files at all. @@ +96,5 @@ > + last if defined $LockFH; > +} > +if( !defined $LockFH ) { > + print "WARNING: Could not create lock file $lockfile: $!\n"; > + print "Please check your koha-conf.xml for ZEBRA_LOCKDIR.\n"; ZEBRA_LOCKDIR should be cron_lockdir here ;) @@ +98,5 @@ > +if( !defined $LockFH ) { > + print "WARNING: Could not create lock file $lockfile: $!\n"; > + print "Please check your koha-conf.xml for ZEBRA_LOCKDIR.\n"; > + print "Verify file permissions for it too.\n"; > + $use_flock = 0; # we disable file locking now and will continue As above, we're not intending on allowing use of this script without lock files after this patch.. I might get a second qa opinion on that, but I can't currently see a reason why we'd need to support a no-lock version.
Hmm.. on second thoughts I think this needs a bit more thought around how it may need to handle the 'type' argument that can be passed to the script.. I can see cases where one would want multiple runs of the script running side by side with distinct caller options. I'll ponder that a little and perhaps add it as a qa followup.
Sorry chaps, I get a `sha1 information is lacking or useless` error when trying to apply this patchset.. seems to be the second patch it falls over on and a manual `patch -p1` < also fails Marking as 'Patch doesn't apply' for now.
(In reply to Martin Renvoize from comment #33) > Hmm.. on second thoughts I think this needs a bit more thought around how it > may need to handle the 'type' argument that can be passed to the script.. I > can see cases where one would want multiple runs of the script running side > by side with distinct caller options. > > I'll ponder that a little and perhaps add it as a qa followup. Yes I agree, we should have a more flexible and generic approach. We already handle similar race conditions for circulation notices (CHECKIN/OUT) in C4::Circulation::SendCirculationAlert The algorithm would be: Get the notices to send for each notice: start a transaction lock the table if the notice needs to be send (fetch the status): send the notice update the status release the lock commit the transaction else: # We certainly faced a race release the lock rollback # certainly useless as we did not modify anything
I never did manage to apply this, but I think it's just a peculiarity with my own git repository. Setting back to Signed Off.. hopefully we can get some more eyes on this again now.. I certainly agree with the principal, though Jonathan raised an alternative approach I hadn't considered. Pondering.
QA: Looking here a bit
Patch does not apply btw.
Hi Marcel Thanks for testing. Can you please tell me which patch is failing to apply? When I test applying on up to date master it is applying fine: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 15529 - Locking process_message_queue.pl cron whilst it is running Applying: Bug 15529 - Wrapping email sending up in the verbose flag Using index info to reconstruct a base tree... M misc/cronjobs/process_message_queue.pl Falling back to patching base and 3-way merge... Auto-merging misc/cronjobs/process_message_queue.pl Applying: Bug 15529: Followup to send without more libraries Applying: Bug 15529: Altered logic for lockfile creation Cheers, Alex
+print "$0 is already running. Exiting.\n"; +#Send email to inform administrator another process_message_queue.pl cron was attempted to run whilst previous execution of the file is taking place. +# Configure accordingly +MIME::Lite->send( 'smtp', ''); +my $email = MIME::Lite->new( +To => ' ', +From => ' ', +Type => 'TEXT', +Subject => 'process_message_queue.pl cron is already running', +Data => 'process_message_queue.pl cron is already running', +); + +$email->attach( +Type => 'Text', + Data => 'process_message_queue.pl cron is already running', ); +$email->send(); +exit(1); +} I would say: Just print already running to STDERR and quit. No need to send email. If the administrator wants an email, he can add MAILTO=root in cron. No need to add MIME::Lite etc.
(In reply to Alex Buckley from comment #39) > Hi Marcel > > Thanks for testing. Can you please tell me which patch is failing to apply? > > When I test applying on up to date master it is applying fine: > > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 15529 - Locking process_message_queue.pl cron whilst it is > running > Applying: Bug 15529 - Wrapping email sending up in the verbose flag > Using index info to reconstruct a base tree... > M misc/cronjobs/process_message_queue.pl > Falling back to patching base and 3-way merge... > Auto-merging misc/cronjobs/process_message_queue.pl > Applying: Bug 15529: Followup to send without more libraries > Applying: Bug 15529: Altered logic for lockfile creation > > Cheers, > Alex Second patch
+my $instancemessagequeuefile = $logdir . "/messagequeue.lock"; +open(FOO, ">> $instancemessagequeuefile") or die "Cannot write to $instancemessagequeuefile: $!"; As Martin already said, the logdir might not be the best candidate. Also look at locking in rebuild_zebra.pl. Note that it tries three places and continues even if the system does not facilitate locking. I see in the follow-ups that you change it. Fine. You now copy locking from rebuild_zebra. Please consolidate: move it to a module.
-C4::Letters::SendQueuedMessages( - { - verbose => $verbose, - username => $username, - password => $password, - method => $method, - limit => $limit, - type => $type, - } +C4::Letters::SendQueuedMessages( { verbose => $verbose, username => $username, password => $password, method => $method } ); +sleep(15); Why do you remove limit and type? Why add sleep (debug leftover)?
(In reply to Marcel de Rooy from comment #43) > -C4::Letters::SendQueuedMessages( > - { > - verbose => $verbose, > - username => $username, > - password => $password, > - method => $method, > - limit => $limit, > - type => $type, > - } > +C4::Letters::SendQueuedMessages( { verbose => $verbose, username => > $username, password => $password, method => $method } ); > +sleep(15); > Why do you remove limit and type? Why add sleep (debug leftover)? I too thought it was a bad thing, which is why the third patch fixes it. However, I thought I patched up the ZEBRA stuff.
Patch no longer applies - is somebody able to rebase? Applying: Bug 15529 - Locking process_message_queue.pl cron whilst it is running Applying: Bug 15529 - Wrapping email sending up in the verbose flag error: sha1 information is lacking or useless (misc/cronjobs/process_message_queue.pl). error: could not build fake ancestor Patch failed at 0001 Bug 15529 - Wrapping email sending up in the verbose flag
Comment on attachment 80256 [details] [review] Bug 15529: Altered logic for lockfile creation Review of attachment 80256 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/process_message_queue.pl @@ +89,5 @@ > +foreach ( > + C4::Context->config("cron_lockdir"), > + '/var/lock/cron_' . C4::Context->config('database'), > + '/tmp/cron_' . C4::Context->config('database') > +) { This seems excessive and process_message_queue.pl doesn't seem like the place you'd want to do it anyway. This seems pretty generic and like it could be used by other scripts. @@ +98,5 @@ > +if( !defined $LockFH ) { > + print "WARNING: Could not create lock file $lockfile: $!\n"; > + print "Please check your koha-conf.xml for ZEBRA_LOCKDIR.\n"; > + print "Verify file permissions for it too.\n"; > + $use_flock = 0; # we disable file locking now and will continue Yeah, I'd say mandate that locks must be used.
*** This bug has been marked as a duplicate of bug 31342 ***
I decided to take another approach as back in 2020 bug 25109 was pushed (for a different script that needed a locking mechanism), and I wasn't aware of the discussion here. Thanks to everyone that invested time on this matter.