[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Jonathan Druart  changed:

   What|Removed |Added

 Blocks||27707


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27707
[Bug 27707] Renewing doesn't work when renewal notices are enabled
-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Joonas Kylmälä  changed:

   What|Removed |Added

   See Also||https://bugs.koha-community
   ||.org/bugzilla3/show_bug.cgi
   ||?id=27707

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #22 from Mason James  ---
(In reply to Julian Maurice from comment #13)
> Pushed to 3.22.x, will be in 3.22.19

Pushed to 16.05.x, for 16.05.12 release

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #21 from Jonathan Druart  
---
(In reply to Marcel de Rooy from comment #20)
> (In reply to David Cook from comment #19)
> > After all, the message_queue should just be FIFO, right? Once it's in there,
> > it's in there as a finished product ready to go out.
> 
> Handling digests differently doesnt sound bad to me.

Well, the way digests are handled is pretty bad IMO.
But anyway we are looking for a quick solution to fix an important issue, not
rethinking the way we are sending messages.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #20 from Marcel de Rooy  ---
(In reply to David Cook from comment #19)
> After all, the message_queue should just be FIFO, right? Once it's in there,
> it's in there as a finished product ready to go out.

Handling digests differently doesnt sound bad to me.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #19 from David Cook  ---
Why aren't we logging individual events (e.g. checkout, checkin, renew, etc),
and then bundling them together before sending out notices if they're digests?

If it's not a digest, you could log it and run generate_notice which puts it
into a mail queue.

If it is a digest, you could log it with a digest/delay flag. Then periodically
(e.g. every fifteen minutes), you could group_by to get the patron ids for all
digests, then iterate through each patron while getting all the digest events
for that patron, and then queue an email. 

process_message_queue could then just run like normal.

I think that would be a lot simpler than using locks. 

After all, the message_queue should just be FIFO, right? Once it's in there,
it's in there as a finished product ready to go out.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

David Cook  changed:

   What|Removed |Added

 CC||dc...@prosentient.com.au

--- Comment #18 from David Cook  ---
(In reply to Jonathan Druart from comment #5)
> Anyone familiar with lock, deadlock and transaction?

In PostgreSQL but not with MySQL.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #17 from Jonathan Druart  
---
(In reply to Marcel de Rooy from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > Stable releases should be safe, SendCirculationAlert is not covered by tests
> 
> It may not be called directly from a test.
> But what about indirect calls? Maybe from AddReturn or AddRenewal or
> AddIssue ?

AFAIK we do not have tests covering messaging preferences.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #16 from Marcel de Rooy  ---
(In reply to Jonathan Druart from comment #15)
> Stable releases should be safe, SendCirculationAlert is not covered by tests

It may not be called directly from a test.
But what about indirect calls? Maybe from AddReturn or AddRenewal or AddIssue ?

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #15 from Jonathan Druart  
---
(In reply to Jonathan Druart from comment #14)
> " LOCK TABLES is not transaction-safe and implicitly commits any active
> transaction before attempting to lock the tables. "
> 
> This is bad for tests...

Will need to be fixed, see bug 18364.
Stable releases should be safe, SendCirculationAlert is not covered by tests

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Jonathan Druart  changed:

   What|Removed |Added

 Blocks||18364


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364
[Bug 18364] LOCK and UNLOCK are not transaction-safe
-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #14 from Jonathan Druart  
---
" LOCK TABLES is not transaction-safe and implicitly commits any active
transaction before attempting to lock the tables. "

This is bad for tests...

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

--- Comment #13 from Julian Maurice  ---
Pushed to 3.22.x, will be in 3.22.19

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Katrin Fischer  changed:

   What|Removed |Added

 Status|Pushed to Master|Pushed to Stable

--- Comment #12 from Katrin Fischer  ---
These patches have been pushed to 16.11.x and will be in 16.11.06
.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Brendan Gallagher  changed:

   What|Removed |Added

 Status|Passed QA   |Pushed to Master
 CC||bren...@bywatersolutions.co
   ||m

--- Comment #11 from Brendan Gallagher  ---
Pushed to Master - Should be in the 17.05 release.  Thanks!

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Martin Renvoize  changed:

   What|Removed |Added

 Status|Signed Off  |Passed QA

--- Comment #10 from Martin Renvoize  ---
Well done Jonathan, not a simple one there.

Passing QA.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Martin Renvoize  changed:

   What|Removed |Added

  Attachment #60814|0   |1
is obsolete||

--- Comment #9 from Martin Renvoize  ---
Created attachment 61422
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61422=edit
Bug 15854: Use a READ and WRITE LOCK on message_queue

To make sure we will not never get a race conditions for these kinds of
notices, we need to add a LOCK on the message_queue table.

This does not smell the best way to do that, but I faced deadlock issues
when I tried to use "UPDATE FOR"

https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
https://dev.mysql.com/doc/refman/5.7/en/commit.html

To test this patch, or another solution, you need to apply manually this
change:

 my $message = C4::Message->find_last_message($borrower, $type, $mtt);
 unless ( $message ) {
+sleep(1);
 C4::Message->enqueue($letter, $borrower, $mtt);
 } else {

And repeat the test plan from first patch.
Do not forget to truncate the message_queue table.

Followed test plans, works as expected.
Signed-off-by: Marc Véron 

Signed-off-by: Martin Renvoize 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Martin Renvoize  changed:

   What|Removed |Added

  Attachment #60813|0   |1
is obsolete||

--- Comment #8 from Martin Renvoize  ---
Created attachment 61421
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61421=edit
Bug 15854: Simplify the code to limit race conditions

There is an obvious race condition when CHECKIN and RENEWAL are
generated from circulation.pl calling svc/renew or svc/checkin in AJAX.

The 2 first queries will try to get the id of the last message
(find_last_message) and if it does not exist, they will insert it.
Theorically that could be lead to have several "digest" messages for a
given patron.
I did not recreate more than 2 messages, from the third one at least one
of the two firsts existed in the DB already.

This patch just simplifies the code to make the SELECT and INSERT or
UPDATE closer and limit the race condition possibilities.

Test plan:
0. Set RenewalSendNotice and circ rules to have a lot of renewals available
1. Use batch checkouts (or one by one) to check out several items to a
patron
2. Empty message_queue (at least of this patron)
3. Renew them all at once ("select all" link, "renew or check in"
button)
4. Check the message_queue
Without this patch you have lot of chances to faced a race condition and
get at least 2 messages for the same patron. This is not expected, we
expect 1 digest with all the messages.
With this patch apply you have lot of chances not to face it, but it's
not 100% safe as we do not use a mechanism to lock the table at the DBMS
level.

Tested both patches together, works as expected.
Signed-off-by: Marc Véron 

Signed-off-by: Martin Renvoize 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Marc Véron  changed:

   What|Removed |Added

 CC||ve...@veron.ch
 Status|Needs Signoff   |Signed Off

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Marc Véron  changed:

   What|Removed |Added

  Attachment #60058|0   |1
is obsolete||

--- Comment #7 from Marc Véron  ---
Created attachment 60814
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60814=edit
Bug 15854: Use a READ and WRITE LOCK on message_queue

To make sure we will not never get a race conditions for these kinds of
notices, we need to add a LOCK on the message_queue table.

This does not smell the best way to do that, but I faced deadlock issues
when I tried to use "UPDATE FOR"

https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
https://dev.mysql.com/doc/refman/5.7/en/commit.html

To test this patch, or another solution, you need to apply manually this
change:

 my $message = C4::Message->find_last_message($borrower, $type, $mtt);
 unless ( $message ) {
+sleep(1);
 C4::Message->enqueue($letter, $borrower, $mtt);
 } else {

And repeat the test plan from first patch.
Do not forget to truncate the message_queue table.

Followed test plans, works as expected.
Signed-off-by: Marc Véron 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Marc Véron  changed:

   What|Removed |Added

  Attachment #60057|0   |1
is obsolete||

--- Comment #6 from Marc Véron  ---
Created attachment 60813
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60813=edit
Bug 15854: Simplify the code to limit race conditions

There is an obvious race condition when CHECKIN and RENEWAL are
generated from circulation.pl calling svc/renew or svc/checkin in AJAX.

The 2 first queries will try to get the id of the last message
(find_last_message) and if it does not exist, they will insert it.
Theorically that could be lead to have several "digest" messages for a
given patron.
I did not recreate more than 2 messages, from the third one at least one
of the two firsts existed in the DB already.

This patch just simplifies the code to make the SELECT and INSERT or
UPDATE closer and limit the race condition possibilities.

Test plan:
0. Set RenewalSendNotice and circ rules to have a lot of renewals available
1. Use batch checkouts (or one by one) to check out several items to a
patron
2. Empty message_queue (at least of this patron)
3. Renew them all at once ("select all" link, "renew or check in"
button)
4. Check the message_queue
Without this patch you have lot of chances to faced a race condition and
get at least 2 messages for the same patron. This is not expected, we
expect 1 digest with all the messages.
With this patch apply you have lot of chances not to face it, but it's
not 100% safe as we do not use a mechanism to lock the table at the DBMS
level.

Tested both patches together, works as expected.
Signed-off-by: Marc Véron 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

[Koha-bugs] [Bug 15854] Race condition for sending renewal/check-in notices

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854

Katrin Fischer  changed:

   What|Removed |Added

Summary|Possible race condition for |Race condition for sending
   |sending renewal/check-in|renewal/check-in notices
   |notices |

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/