From 08ac4b9589a47f1dccba161759ff2f704bf1ef9c Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 4 Dec 2020 12:03:52 +0000 Subject: [PATCH] Bug 26457: [19.11.x] Throw exception if update of issues table fails While this won't prevent the deadlock, it should catch the case where a deadlock causes the DB update to fail and provide feedback to the user and rollback the transaction I don't know how to trigger the deadlock, I can only confirm that we see it, and that this should catch it. To test: 1 - Apply patches 2 - Checkout several items to a patron 3 - Confirm that 'Renew all' feature continues to work as expected and all items are renewed Signed-off-by: David Nind Signed-off-by: Marcel de Rooy Bug 26457: Unit test Signed-off-by: David Nind Signed-off-by: Marcel de Rooy Bug 26457: (QA follow-up) Switch to PK index in UPDATE on issues The deadlock reports tell us that multiple transactions are waiting for a X lock on a record but using a secondary index on borrowernumber and itemnumber. Since we have the issue_id at hand already, we should use that and benefit from the clustered index (on PK) instead of using a secondary index. Signed-off-by: Marcel de Rooy --- C4/Circulation.pm | 17 +++++++++++------ Koha/Exceptions/Checkout.pm | 15 +++++++++++++++ svc/renew | 13 +++++++++++-- t/db_dependent/Circulation.t | 8 +++++++- 4 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 Koha/Exceptions/Checkout.pm diff --git a/C4/Circulation.pm b/C4/Circulation.pm index f03912352b..42e3fb140b 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -61,6 +61,7 @@ use Koha::Config::SysPrefs; use Koha::Charges::Fees; use Koha::Util::SystemPreferences; use Koha::Checkouts::ReturnClaims; +use Koha::Exceptions::Checkout; use Carp; use List::MoreUtils qw( uniq any ); use Scalar::Util qw( looks_like_number ); @@ -2961,13 +2962,17 @@ sub AddRenewal { # Update the issues record to have the new due date, and a new count # of how many times it has been renewed. - my $renews = $issue->renewals + 1; - my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? - WHERE borrowernumber=? - AND itemnumber=?" - ); + my $renews = ( $issue->renewals || 0 ) + 1; + my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? WHERE issue_id = ?"); - $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); + eval{ + $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $issue->issue_id ); + }; + if( $sth->err ){ + Koha::Exceptions::Checkout::FailedRenewal->throw( + error => 'Update of issue# ' . $issue->issue_id . ' failed with error: ' . $sth->errstr + ); + } # Update the renewal count on the item, and tell zebra to reindex $renews = $item_object->renewals + 1; diff --git a/Koha/Exceptions/Checkout.pm b/Koha/Exceptions/Checkout.pm new file mode 100644 index 0000000000..ed4b0a5a27 --- /dev/null +++ b/Koha/Exceptions/Checkout.pm @@ -0,0 +1,15 @@ +package Koha::Exceptions::Checkout; + +use Modern::Perl; + +use Exception::Class ( + 'Koha::Exceptions::Checkout' => { + description => "Something went wrong!" + }, + 'Koha::Exceptions::Checkout::FailedRenewal' => { + isa => 'Koha::Exceptions::Checkout', + description => "Renewing checkout failed" + }, +); + +1; diff --git a/svc/renew b/svc/renew index 29e51d8cf7..99efe2a674 100755 --- a/svc/renew +++ b/svc/renew @@ -21,6 +21,7 @@ use Modern::Perl; use CGI; use JSON qw(to_json); +use Try::Tiny; use C4::Circulation; use C4::Context; @@ -66,8 +67,16 @@ if ( $data->{error} && $data->{error} eq 'on_reserve' && C4::Context->preference } if ( $data->{renew_okay} ) { - $date_due = AddRenewal( $borrowernumber, $itemnumber, $branchcode, $date_due ); - $data->{date_due} = output_pref( { dt => $date_due, as_due_date => 1 } ); + try{ + $date_due = AddRenewal( $borrowernumber, $itemnumber, $branchcode, $date_due ); + $data->{date_due} = output_pref( { dt => $date_due, as_due_date => 1 } ); + } catch { + if ( ref($_) eq 'Koha::Exceptions::Checkout::FailedRenewal' ) { + $data->{error} = 'renewal_failed'; + } else { + $_->rethrow; + } + }; } print to_json($data); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 6d18724b34..182fa3bff4 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -19,6 +19,7 @@ use Modern::Perl; use utf8; use Test::More tests => 47; +use Test::Exception; use Test::MockModule; use Data::Dumper; @@ -3099,7 +3100,7 @@ subtest 'AddReturn should clear items.onloan for unissued items' => sub { subtest 'AddRenewal and AddIssuingCharge tests' => sub { - plan tests => 11; + plan tests => 12; t::lib::Mocks::mock_preference('item-level_itypes', 1); @@ -3142,6 +3143,11 @@ subtest 'AddRenewal and AddIssuingCharge tests' => sub { # Check the item out AddIssue( $patron->unblessed, $item->barcode ); + + throws_ok { + AddRenewal( $patron->borrowernumber, $item->itemnumber, $library->id, undef, {break=>"the_renewal"} ); + } 'Koha::Exceptions::Checkout::FailedRenewal', 'Exception is thrown when renewal update to issues fails'; + t::lib::Mocks::mock_preference( 'RenewalLog', 0 ); my $date = output_pref( { dt => dt_from_string(), dateonly => 1, dateformat => 'iso' } ); my %params_renewal = ( -- 2.20.1