Bugzilla – Attachment 61421 Details for
Bug 15854
Race condition for sending renewal/check-in notices
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15854: Simplify the code to limit race conditions
Bug-15854-Simplify-the-code-to-limit-race-conditio.patch (text/plain), 4.99 KB, created by
Martin Renvoize (ashimema)
on 2017-03-21 19:53:38 UTC
(
hide
)
Description:
Bug 15854: Simplify the code to limit race conditions
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2017-03-21 19:53:38 UTC
Size:
4.99 KB
patch
obsolete
>From 5cd72f86fd8f57f7c8220eba8d5fe3846bb98847 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 9 Feb 2017 12:13:07 +0100 >Subject: [PATCH] Bug 15854: Simplify the code to limit race conditions >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >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 <veron@veron.ch> > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > C4/Circulation.pm | 57 +++++++++++++++++++------------------------------------ > 1 file changed, 20 insertions(+), 37 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index af5acce..5dea088 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3315,7 +3315,7 @@ sub SendCirculationAlert { > my %message_name = ( > CHECKIN => 'Item_Check_in', > CHECKOUT => 'Item_Checkout', >- RENEWAL => 'Item_Checkout', >+ RENEWAL => 'Item_Checkout', > ); > my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences({ > borrowernumber => $borrower->{borrowernumber}, >@@ -3324,43 +3324,26 @@ sub SendCirculationAlert { > my $issues_table = ( $type eq 'CHECKOUT' || $type eq 'RENEWAL' ) ? 'issues' : 'old_issues'; > > my @transports = keys %{ $borrower_preferences->{transports} }; >- # warn "no transports" unless @transports; >- for (@transports) { >- # warn "transport: $_"; >- my $message = C4::Message->find_last_message($borrower, $type, $_); >- if (!$message) { >- #warn "create new message"; >- my $letter = C4::Letters::GetPreparedLetter ( >- module => 'circulation', >- letter_code => $type, >- branchcode => $branch, >- message_transport_type => $_, >- tables => { >- $issues_table => $item->{itemnumber}, >- 'items' => $item->{itemnumber}, >- 'biblio' => $item->{biblionumber}, >- 'biblioitems' => $item->{biblionumber}, >- 'borrowers' => $borrower, >- 'branches' => $branch, >- } >- ) or next; >- C4::Message->enqueue($letter, $borrower, $_); >+ for my $mtt (@transports) { >+ my $letter = C4::Letters::GetPreparedLetter ( >+ module => 'circulation', >+ letter_code => $type, >+ branchcode => $branch, >+ message_transport_type => $mtt, >+ tables => { >+ $issues_table => $item->{itemnumber}, >+ 'items' => $item->{itemnumber}, >+ 'biblio' => $item->{biblionumber}, >+ 'biblioitems' => $item->{biblionumber}, >+ 'borrowers' => $borrower, >+ 'branches' => $branch, >+ } >+ ) or next; >+ >+ my $message = C4::Message->find_last_message($borrower, $type, $mtt); >+ unless ( $message ) { >+ C4::Message->enqueue($letter, $borrower, $mtt); > } else { >- #warn "append to old message"; >- my $letter = C4::Letters::GetPreparedLetter ( >- module => 'circulation', >- letter_code => $type, >- branchcode => $branch, >- message_transport_type => $_, >- tables => { >- $issues_table => $item->{itemnumber}, >- 'items' => $item->{itemnumber}, >- 'biblio' => $item->{biblionumber}, >- 'biblioitems' => $item->{biblionumber}, >- 'borrowers' => $borrower, >- 'branches' => $branch, >- } >- ) or next; > $message->append($letter); > $message->update; > } >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 15854
:
60057
|
60058
|
60813
|
60814
| 61421 |
61422