Bug 31342 - process_message_queue can run over the top of itself causing double-up emails
Summary: process_message_queue can run over the top of itself causing double-up emails
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
: 15529 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-08-11 15:56 UTC by Liz Rea
Modified: 2022-09-19 18:17 UTC (History)
8 users (show)

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


Attachments
Bug 31342: Add execution locking to process_message_queue.pl (1.63 KB, patch)
2022-08-12 09:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31342: Add execution locking to process_message_queue.pl (1.67 KB, patch)
2022-08-12 14:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 31342: Add execution locking to process_message_queue.pl (1.73 KB, patch)
2022-09-16 18:08 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liz Rea 2022-08-11 15:56:33 UTC
IF a process message queue takes too long and happens to hit the next scheduled run, messages will be duplicated and eventually sent twice.

It would be nice to prevent that without changing the cron definition.

Liz

(PS I am aware of the existence of 15529, Tomas asked me to file this new bug as he has a different idea and didn't want to wade into all of the existing drama around the solution on 15529)
Comment 1 Tomás Cohen Arazi 2022-08-12 09:49:57 UTC
*** Bug 15529 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2022-08-12 09:56:55 UTC
Created attachment 139045 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Liz Rea 2022-08-12 14:31:57 UTC
Created attachment 139057 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Liz Rea <liz@bywatersolutions.com>
Comment 4 Marcel de Rooy 2022-08-17 11:30:12 UTC
I do not really mind about pushing this dev. But I wonder if we really address the problem here. Could we do something with status in msg queue itself?

What now might happen, is not getting a lock for some reason resulting in no mails from the system at all.
This sounds worse than receiving two mails to me.
Comment 5 Liz Rea 2022-08-24 14:26:07 UTC
As someone who has experienced both conditions, from the point of view of libraries I've had experience with, they'd rather get no mails than two. 

Double mails says to library patrons "the library doesn't know what they are doing and their system sucks" - no mail says "the internet is glitchy," which is true.
Comment 6 Liz Rea 2022-08-24 14:31:53 UTC
More succinctly: double mail is a PR problem, no mail is less so.
Comment 7 Kyle M Hall 2022-09-16 18:08:58 UTC
Created attachment 140733 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Liz Rea <liz@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Tomás Cohen Arazi 2022-09-19 18:17:55 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!