Bug 15529 - Process Message Queue may send duplicate emails if process is launched twice
Summary: Process Message Queue may send duplicate emails if process is launched twice
Status: RESOLVED DUPLICATE of bug 31342
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-08 14:45 UTC by Barton Chittenden
Modified: 2022-08-12 10:01 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15529 - Using flock to lock new messagequeue.lock lockfile whilst process_message_queue.pl is being executed (2.52 KB, patch)
2018-08-28 00:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl (3.32 KB, patch)
2018-08-28 02:04 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl (3.20 KB, patch)
2018-08-28 02:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Lock /var/lock/koha/<instance>/messagequeue.lock while running process_message_queue.pl (3.37 KB, patch)
2018-08-28 02:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Locking process_message_queue.pl cron whilst it is running (3.84 KB, patch)
2018-09-11 22:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Locking process_message_queue.pl cron whilst it is running (3.94 KB, patch)
2018-09-16 00:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Wrapping email sending up in the verbose flag (3.23 KB, patch)
2018-09-30 09:32 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529: Followup to send without more libraries (4.22 KB, patch)
2018-10-01 14:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15529: Followup to send without more libraries (4.60 KB, patch)
2018-10-01 14:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15529: Followup to send without more libraries (4.70 KB, patch)
2018-10-03 20:53 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15529 - Locking process_message_queue.pl cron whilst it is running (4.00 KB, patch)
2018-10-04 00:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15529 - Wrapping email sending up in the verbose flag (3.29 KB, patch)
2018-10-04 00:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15529: Followup to send without more libraries (4.70 KB, patch)
2018-10-04 00:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15529: Altered logic for lockfile creation (7.57 KB, patch)
2018-10-09 11:25 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2016-01-08 14:45:41 UTC
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.
Comment 1 Liz Rea 2016-02-04 00:13:13 UTC
should set a flock like we did for zebra.
Comment 2 Barton Chittenden 2017-02-01 21:02:43 UTC
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
Comment 3 Katrin Fischer 2017-03-21 06:44:01 UTC
Barton, take a look at:
Bug 15854 - Race condition for sending renewal/check-in notices
This is probably responsible for the multiple messages.
Comment 4 Alex Buckley 2018-08-28 00:39:32 UTC
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
Comment 5 Alex Buckley 2018-08-28 00:40:31 UTC
I have attached a preliminary patch which will be refined further in the near future.
Comment 6 Alex Buckley 2018-08-28 02:04:47 UTC
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
Comment 7 Alex Buckley 2018-08-28 02:08:25 UTC
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
Comment 8 Alex Buckley 2018-08-28 02:39:28 UTC
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 9 paxed 2018-08-28 04:47:09 UTC
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?
Comment 10 Alex Buckley 2018-09-11 22:10:15 UTC
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
Comment 11 Alex Buckley 2018-09-16 00:39:36 UTC
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
Comment 12 Alex Buckley 2018-09-16 00:39:58 UTC
Rebased against master on 16th of sept 2018.
Comment 13 Mark Tompsett 2018-09-24 17:16:44 UTC
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?
Comment 14 Alex Buckley 2018-09-30 09:32:09 UTC
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
Comment 15 Alex Buckley 2018-09-30 09:37:14 UTC
(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.
Comment 16 Mark Tompsett 2018-09-30 12:39:02 UTC
Then why not trigger the process_messages.pl immediately after queuing? Adding more libraries is still very ugly.

my $return = qx{echo "foobar"};
Comment 17 Mark Tompsett 2018-10-01 13:51:07 UTC
How about this?
Comment 18 Mark Tompsett 2018-10-01 14:32:19 UTC
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.
Comment 19 Mark Tompsett 2018-10-01 14:32:51 UTC
Apply ALL the patches.
Comment 20 Mark Tompsett 2018-10-01 14:46:36 UTC
Comment on attachment 79719 [details] [review]
Bug 15529: Followup to send without more libraries

Forgot to git commit changes first. :)
Comment 21 Mark Tompsett 2018-10-01 14:47:50 UTC
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.
Comment 22 Alex Buckley 2018-10-03 20:52:23 UTC
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
Comment 23 Alex Buckley 2018-10-03 20:53:24 UTC
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>
Comment 24 Mark Tompsett 2018-10-04 00:02:05 UTC
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>
Comment 25 Mark Tompsett 2018-10-04 00:02:09 UTC
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>
Comment 26 Mark Tompsett 2018-10-04 00:02:13 UTC
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>
Comment 27 Mark Tompsett 2018-10-04 00:03:13 UTC
I signed off Alex's having tested with all three. Alex has signed off mine having tests with all three. Marking as signed off.
Comment 28 Martin Renvoize 2018-10-09 09:18:51 UTC
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.
Comment 29 Martin Renvoize 2018-10-09 09:20:33 UTC
See bug 11078 or the current rebuild_zebra.pl script for inspirations perhaps ;)
Comment 30 Alex Buckley 2018-10-09 11:25:15 UTC
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
Comment 31 Alex Buckley 2018-10-09 11:27:41 UTC
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 32 Martin Renvoize 2018-10-09 12:13:42 UTC
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.
Comment 33 Martin Renvoize 2018-10-09 13:07:19 UTC
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.
Comment 34 Martin Renvoize 2018-10-09 13:14:46 UTC
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.
Comment 35 Jonathan Druart 2018-10-09 15:17:35 UTC
(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
Comment 36 Martin Renvoize 2018-10-12 14:09:11 UTC
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.
Comment 37 Marcel de Rooy 2018-11-02 07:58:43 UTC
QA: Looking here a bit
Comment 38 Marcel de Rooy 2018-11-02 08:10:51 UTC
Patch does not apply btw.
Comment 39 Alex Buckley 2018-11-02 08:19:15 UTC
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
Comment 40 Marcel de Rooy 2018-11-02 08:24:50 UTC
+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.
Comment 41 Marcel de Rooy 2018-11-02 08:25:13 UTC
(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
Comment 42 Marcel de Rooy 2018-11-02 08:26:34 UTC
+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.
Comment 43 Marcel de Rooy 2018-11-02 08:27:03 UTC
-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)?
Comment 44 Mark Tompsett 2018-11-02 12:57:48 UTC
(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.
Comment 45 Hayley Pelham 2019-03-13 03:27:18 UTC
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 46 David Cook 2019-03-13 04:03:48 UTC
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.
Comment 47 Tomás Cohen Arazi 2022-08-12 09:49:57 UTC

*** This bug has been marked as a duplicate of bug 31342 ***
Comment 48 Tomás Cohen Arazi 2022-08-12 10:01:21 UTC
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.