Serials: MFHD::get_compressed_holdings() can reach infinite loop

Even controlled serials holdings involve the internal creation of MFHD
fields, upon which caculations are performed for such purposes as the
display of holdings summaries in the OPAC.

There are too many ways that incorrect MFHD (or MFHD that our code just
can't yet handle) can lead our MFHD routines to crash. We can't address
all these possibilities in a single bug fix. But we can avoid this
infinite loop.

A subroutine within open-ils.serial (_summarize_contents()) relies on
MFHD::get_compressed_holdings(). When the latter went into an infinite
loop the result would be an open-il.serial drone process consuming CPU
time indefinitely and, depending on the data that provoked the loop,
potentially writing repeating messages to stderr indefinitely.

End users will still see the item receiving fail in these cases, and be
obliged to work around the issue as before until more robust
holdings summarization code can be written, but at least the overall
condition of the running Evergreen system won't be affected, and there
will be better information in the error logs.

Signed-off-by: Lebbeous Fogle-Weekley <lebbeous@esilibrary.com>
Signed-off-by: Remington Steed <rjs7@calvin.edu>
Signed-off-by: Dan Wells <dbw2@calvin.edu>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Serial.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Serial.pm
index 19b14aa..6425257 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Serial.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Serial.pm
@@ -644,6 +644,9 @@ sub scoped_bib_holdings_summary {
     my %statement_blob;
     for my $type ( keys %type_blob ) {
         my ($mfhd,$list) = _summarize_contents(new_editor(), $type_blob{$type});
+
+        return {} if $U->event_code($mfhd); # _summarize_contents() failed, bad data?
+
         $statement_blob{$type} = $list;
     }
 
@@ -1496,6 +1499,7 @@ sub _prepare_unit {
     }
 
     my ($mfhd, $formatted_parts) = _summarize_contents($e, $issuances);
+    return $mfhd if $U->event_code($mfhd);
 
     # special case for single formatted_part (may have summarized version)
     if (@$formatted_parts == 1) {
@@ -1527,6 +1531,7 @@ sub _prepare_summaries {
     my ($e, $issuances, $sdist, $type) = @_;
 
     my ($mfhd, $formatted_parts) = _summarize_contents($e, $issuances, $sdist);
+    return $mfhd if $U->event_code($mfhd);
 
     my $search_method = "search_serial_${type}_summary";
     my $summary = $e->$search_method([{"distribution" => $sdist->id}]);
@@ -1812,7 +1817,6 @@ sub _build_unit {
     return $unit;
 }
 
-
 sub _summarize_contents {
     my $editor = shift;
     my $issuances = shift;
@@ -1891,11 +1895,24 @@ sub _summarize_contents {
 
     my @formatted_parts;
     my @scap_fields_ordered = $mfhd->field('85[345]');
+
     foreach my $scap_field (@scap_fields_ordered) { #TODO: use generic MFHD "summarize" method, once available
-       my @updated_holdings = $mfhd->get_compressed_holdings($scap_field);
-       foreach my $holding (@updated_holdings) {
-           push(@formatted_parts, $holding->format);
-       }
+        my @updated_holdings;
+        eval {
+            @updated_holdings = $mfhd->get_compressed_holdings($scap_field);
+        };
+        if ($@) {
+            my $msg = "get_compressed_holdings(): $@ ; using sdist ID #" .
+                ($sdist ? $sdist->id : "<NONE>") . " and " .
+                scalar(@$issuances) . " issuances, of which one has ID #" .
+                $issuances->[0]->id;
+
+            $msg =~ s/\n//gm;
+            $logger->error($msg);
+            return new OpenILS::Event("BAD_PARAMS", note => $msg);
+        }
+
+        push @formatted_parts, map { $_->format } @updated_holdings;
     }
 
     return ($mfhd, \@formatted_parts);

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Utils/MFHD.pm b/Open-ILS/src/perlmods/lib/OpenILS/Utils/MFHD.pm
index 2bf94d4..32ea066 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Utils/MFHD.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Utils/MFHD.pm
@@ -404,11 +404,23 @@ sub get_compressed_holdings {
             last;
         } else {
             push(@comp_holdings, $curr_holding);
+
+            my $loop_count = 0;
+            (my $runner_dump = $runner->as_formatted) =~ s/\n\s+/*/gm; # logging
+
             while ($runner le $holding) {
-                # Here is where we used to get stuck in an infinite loop
-                # until the "Don't know how to deal with frequency" was
-                # elevated from a carp to a croak.
+                # Infinite loops used to happen here. As written today,
+                # ->increment() cannot be guaranteed to eventually falsify
+                # the condition ($runner le $holding) in certain cases.
+
                 $runner->increment;
+
+                if (++$loop_count >= 10000) {
+                    (my $holding_dump = $holding->as_formatted) =~ s/\n\s+/*/gm;
+
+                    croak "\$runner<$runner_dump> didn't catch up with " .
+                        "\$holding<$holding_dump> after 10000 increments";
+                }
             }
             $curr_holding = $holding->clone;
             $seqno++;