Bugzilla – Attachment 194990 Details for
Bug 41801
FixPriority recursive calls for lowestPriority holds can be removed
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41801: (QA follow-up) Prevent race condition with txn_do and FOR UPDATE
f27a55a.patch (text/plain), 9.51 KB, created by
Martin Renvoize (ashimema)
on 2026-03-09 10:37:59 UTC
(
hide
)
Description:
Bug 41801: (QA follow-up) Prevent race condition with txn_do and FOR UPDATE
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-09 10:37:59 UTC
Size:
9.51 KB
patch
obsolete
>From f27a55aeb0eb5a437e422669021bce4d2de65374 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Mon, 9 Mar 2026 10:13:33 +0000 >Subject: [PATCH] Bug 41801: (QA follow-up) Prevent race condition with txn_do > and FOR UPDATE >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >FixPriority has a read-modify-write structure: it SELECTs all active >holds for a biblionumber, computes new priority order in Perl, then >issues N UPDATEs. Two concurrent callers (e.g. two patrons adjusting >holds on a popular title at the same moment) could both read the same >snapshot and then overwrite each other's renumbering, leaving duplicate >or incorrect priority values. > >Fix by: > >1. Wrapping the entire function body in txn_do so it runs atomically. > txn_do uses MySQL savepoints when called inside an existing > transaction, so callers that already hold a transaction are unaffected. > >2. Adding FOR UPDATE to the initial SELECT so the first caller > acquires row-level locks on all holds for that bib. A second > concurrent caller will block at the SELECT until the first commits, > then reads the already-corrected priorities and produces a consistent > result. > >Note: FOR UPDATE locks only rows that exist at read time; a concurrent >INSERT of a brand-new hold can still slip through, but priority >assignment for new holds goes via AddReserve รข FixPriority, which will >also acquire the lock and renumber correctly. >--- > C4/Reserves.pm | 177 +++++++++++++++++++++++++------------------------ > 1 file changed, 92 insertions(+), 85 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index fe555348490..8a7f13bc092 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -1662,101 +1662,108 @@ priority adjusted to ensure that they remain at the end of the line. > =cut > > sub FixPriority { >- my ($params) = @_; >- my $reserve_id = $params->{reserve_id}; >- my $rank = $params->{rank} // ''; >- my $biblionumber = $params->{biblionumber}; >- >- my $dbh = C4::Context->dbh; >+ my ($params) = @_; > >- my $hold; >- if ($reserve_id) { >- $hold = Koha::Holds->find($reserve_id); >- if ( !defined $hold ) { >+ Koha::Database->new->schema->txn_do( >+ sub { >+ my $reserve_id = $params->{reserve_id}; >+ my $rank = $params->{rank} // ''; >+ my $biblionumber = $params->{biblionumber}; > >- # may have already been checked out and hold fulfilled >- require Koha::Old::Holds; >- $hold = Koha::Old::Holds->find($reserve_id); >- } >- return unless $hold; >- } >+ my $dbh = C4::Context->dbh; > >- unless ($biblionumber) { # FIXME This is a very weird API >- $biblionumber = $hold->biblionumber; >- } >+ my $hold; >+ if ($reserve_id) { >+ $hold = Koha::Holds->find($reserve_id); >+ if ( !defined $hold ) { > >- if ( $rank eq "del" ) { # FIXME will crash if called without $hold >- $hold->cancel; >- } elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) { >- >- # make sure priority for waiting or in-transit items is 0 >- my $query = " >- UPDATE reserves >- SET priority = 0 >- WHERE reserve_id = ? >- AND found IN ('W', 'T', 'P') >- "; >- my $sth = $dbh->prepare($query); >- $sth->execute($reserve_id); >- } >- my @priority; >+ # may have already been checked out and hold fulfilled >+ require Koha::Old::Holds; >+ $hold = Koha::Old::Holds->find($reserve_id); >+ } >+ return unless $hold; >+ } > >- # get what's left, sorting lowestPriority holds to the bottom >- my $query = " >- SELECT reserve_id, borrowernumber, reservedate, lowestPriority >- FROM reserves >- WHERE biblionumber = ? >- AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL) >- ORDER BY lowestPriority ASC, priority ASC >- "; >- my $sth = $dbh->prepare($query); >- $sth->execute($biblionumber); >- while ( my $line = $sth->fetchrow_hashref ) { >- push( @priority, $line ); >- } >+ unless ($biblionumber) { # FIXME This is a very weird API >+ $biblionumber = $hold->biblionumber; >+ } > >- # FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given >- # To find the matching index >- my $i; >- my $key = -1; # to allow for 0 to be a valid result >- for ( $i = 0 ; $i < @priority ; $i++ ) { >- if ( $reserve_id && $reserve_id == $priority[$i]->{'reserve_id'} ) { >- $key = $i; # save the index >- last; >- } >- } >+ if ( $rank eq "del" ) { # FIXME will crash if called without $hold >+ $hold->cancel; >+ } elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) { >+ >+ # make sure priority for waiting or in-transit items is 0 >+ my $query = " >+ UPDATE reserves >+ SET priority = 0 >+ WHERE reserve_id = ? >+ AND found IN ('W', 'T', 'P') >+ "; >+ my $sth = $dbh->prepare($query); >+ $sth->execute($reserve_id); >+ } >+ my @priority; >+ >+ # Lock all active holds for this bib so concurrent FixPriority calls >+ # for the same hold set cannot interleave their read-modify-write cycle. >+ # FOR UPDATE is effective inside the enclosing txn_do transaction. >+ my $query = " >+ SELECT reserve_id, borrowernumber, reservedate, lowestPriority >+ FROM reserves >+ WHERE biblionumber = ? >+ AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL) >+ ORDER BY lowestPriority ASC, priority ASC >+ FOR UPDATE >+ "; >+ my $sth = $dbh->prepare($query); >+ $sth->execute($biblionumber); >+ while ( my $line = $sth->fetchrow_hashref ) { >+ push( @priority, $line ); >+ } > >- # if this hold is marked lowest priority, we can only move it so far; >- # cap rank to just after the last non-lowestPriority hold using the >- # already-fetched @priority array (avoids a second DB query and stale data) >- if ( $hold && $hold->lowestPriority && $rank ne 'del' && $rank > 0 ) { >- my $non_lowest_count = scalar grep { !$_->{lowestPriority} } @priority; >- $rank = $non_lowest_count + 1 if $non_lowest_count && $rank <= $non_lowest_count; >- } >+ # FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given >+ # To find the matching index >+ my $i; >+ my $key = -1; # to allow for 0 to be a valid result >+ for ( $i = 0 ; $i < @priority ; $i++ ) { >+ if ( $reserve_id && $reserve_id == $priority[$i]->{'reserve_id'} ) { >+ $key = $i; # save the index >+ last; >+ } >+ } > >- # if index exists in array then move it to new position >- if ( $key > -1 && $rank ne 'del' && $rank > 0 ) { >- my $new_rank = $rank - 1; # $new_rank is what you want the new index to be in the array >- my $moving_item = splice( @priority, $key, 1 ); >- $new_rank = scalar @priority if $new_rank > scalar @priority; >- splice( @priority, $new_rank, 0, $moving_item ); >- } >+ # if this hold is marked lowest priority, we can only move it so far; >+ # cap rank to just after the last non-lowestPriority hold using the >+ # already-fetched @priority array (avoids a second DB query and stale data) >+ if ( $hold && $hold->lowestPriority && $rank ne 'del' && $rank > 0 ) { >+ my $non_lowest_count = scalar grep { !$_->{lowestPriority} } @priority; >+ $rank = $non_lowest_count + 1 if $non_lowest_count && $rank <= $non_lowest_count; >+ } > >- # now fix the priority on those that are left.... >- # only updating if changed >- $query = " >- UPDATE reserves >- SET priority = ? >- WHERE reserve_id = ? AND priority != ? >- "; >- $sth = $dbh->prepare($query); >- for ( my $j = 0 ; $j < @priority ; $j++ ) { >- $sth->execute( >- $j + 1, >- $priority[$j]->{'reserve_id'}, $j + 1 >- ); >- } >+ # if index exists in array then move it to new position >+ if ( $key > -1 && $rank ne 'del' && $rank > 0 ) { >+ my $new_rank = $rank - 1; # $new_rank is what you want the new index to be in the array >+ my $moving_item = splice( @priority, $key, 1 ); >+ $new_rank = scalar @priority if $new_rank > scalar @priority; >+ splice( @priority, $new_rank, 0, $moving_item ); >+ } > >+ # now fix the priority on those that are left.... >+ # only updating if changed >+ $query = " >+ UPDATE reserves >+ SET priority = ? >+ WHERE reserve_id = ? AND priority != ? >+ "; >+ $sth = $dbh->prepare($query); >+ for ( my $j = 0 ; $j < @priority ; $j++ ) { >+ $sth->execute( >+ $j + 1, >+ $priority[$j]->{'reserve_id'}, $j + 1 >+ ); >+ } >+ } >+ ); > } > > =head2 _Findgroupreserve >-- >2.53.0 >
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 41801
:
192820
|
194985
|
194986
|
194987
|
194988
|
194989
| 194990