Bug 15006: Centralize timeout logic and allow zero client timeout

Moving timeout logic to one routine (with unit test).

This further implements two suggestions from Kyle and Larry:

[1] You could use a client_timeout of 0 to specify no timeout at all.
[2] Have the client_timeout default to the timeout if not defined.

Test plan:
[1] Run t/db_dependent/SIP/SIPServer.t.
[2] Test login timeout for raw and telnet.
[3] Check ACS status message for timeout value. Should match policy
    timeout from institution.
[4] Test client timeout (zero and non-zero).
[5] Remove client timeout. Test fallback to service.
[6] Remove service timeout too. Test fallback to 30 at login.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended to incorporate Srdjan's suggestion to move get_timeout to
SIPServer.pm; this requires some additional mocking in the unit test.
And even makes the test db dependent, as documented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm
index 0a76ca8..ea7bf7b 100755 (executable)
--- a/C4/SIP/SIPServer.pm
+++ b/C4/SIP/SIPServer.pm
@@ -135,8 +135,9 @@ sub raw_transport {
     # Timeout the while loop if we get stuck in it
     # In practice it should only iterate once but be prepared
     local $SIG{ALRM} = sub { die 'raw transport Timed Out!' };
-    syslog('LOG_DEBUG', "raw_transport: timeout is $service->{timeout}");
-    alarm $service->{timeout};
+    my $timeout = $self->get_timeout({ transport => 1 });
+    syslog('LOG_DEBUG', "raw_transport: timeout is $timeout");
+    alarm $timeout;
     while (!$self->{account}) {
         $input = read_request();
         if (!$input) {
@@ -193,8 +194,8 @@ sub telnet_transport {
     my $account = undef;
     my $input;
     my $config  = $self->{config};
-       my $timeout = $self->{service}->{timeout} || $config->{timeout} || 30;
-       syslog("LOG_DEBUG", "telnet_transport: timeout is %s", $timeout);
+    my $timeout = $self->get_timeout({ transport => 1 });
+    syslog("LOG_DEBUG", "telnet_transport: timeout is $timeout");
 
     eval {
        local $SIG{ALRM} = sub { die "telnet_transport: Timed Out ($timeout seconds)!\n"; };
@@ -256,7 +257,7 @@ sub sip_protocol_loop {
     my $self = shift;
     my $service = $self->{service};
     my $config  = $self->{config};
-    my $timeout = $self->{service}->{timeout} || $config->{timeout} || 30;
+    my $timeout = $self->get_timeout({ client => 1 });
 
     # The spec says the first message will be:
     #     SIP v1: SC_STATUS
@@ -343,5 +344,49 @@ sub read_request {
       syslog( 'LOG_INFO', "INPUT MSG: '$buffer'" );
       return $buffer;
 }
+
+# $server->get_timeout({ $type => 1, fallback => $fallback });
+#     where $type is transport | client | policy
+#
+# Centralizes all timeout logic.
+# Transport refers to login process, client to active connections.
+# Policy timeout is transaction timeout (used in ACS status message).
+#
+# Fallback is optional. If you do not pass transport, client or policy,
+# you will get fallback or hardcoded default.
+
+sub get_timeout {
+    my ( $server, $params ) = @_;
+    my $fallback = $params->{fallback} || 30;
+    my $service = $server->{service} // {};
+    my $config = $server->{config} // {};
+
+    if( $params->{transport} ||
+        ( $params->{client} && !exists $service->{client_timeout} )) {
+        # We do not allow zero values here.
+        # Note: config/timeout seems to be deprecated.
+        return $service->{timeout} || $config->{timeout} || $fallback;
+
+    } elsif( $params->{client} ) {
+        # We know that client_timeout exists now.
+        # We do allow zero values here to indicate no timeout.
+        return 0 if $service->{client_timeout} =~ /^0+$|\D/;
+        return $service->{client_timeout};
+
+    } elsif( $params->{policy} ) {
+        my $policy = $server->{policy} // {};
+        my $rv = sprintf( "%03d", $policy->{timeout} // 0 );
+        if( length($rv) != 3 ) {
+            syslog( "LOG_ERR", "Policy timeout has wrong size: '%s'", $rv );
+            return '000';
+        }
+        return $rv;
+
+    } else {
+        return $fallback;
+    }
+}
+
 1;
+
 __END__

diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
index 787449b..4e406e2 100644 (file)
--- a/C4/SIP/Sip/MsgType.pm
+++ b/C4/SIP/Sip/MsgType.pm
@@ -1505,14 +1505,9 @@ sub send_acs_status {
     $ACS_renewal_policy = sipbool( $policy->{renewal} );
     $status_update_ok   = sipbool( $ils->status_update_ok );
     $offline_ok         = sipbool( $ils->offline_ok );
-    $timeout            = sprintf( "%03d", $policy->{timeout} );
+    $timeout            = $server->get_timeout({ policy => 1 });
     $retries            = sprintf( "%03d", $policy->{retries} );
 
-    if ( length($timeout) != 3 ) {
-        syslog( "LOG_ERR", "handle_acs_status: timeout field wrong size: '%s'", $timeout );
-        $timeout = '000';
-    }
-
     if ( length($retries) != 3 ) {
         syslog( "LOG_ERR", "handle_acs_status: retries field wrong size: '%s'", $retries );
         $retries = '000';

diff --git a/t/SIP/Sip.t b/t/SIP/Sip.t
new file mode 100755 (executable)
index 0000000..f670277
--- /dev/null
+++ b/t/SIP/Sip.t
@@ -0,0 +1,58 @@
+#!/usr/bin/perl
+
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# Koha is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Koha; if not, see <http://www.gnu.org/licenses>.
+
+use Modern::Perl;
+
+use Test::More tests => 9;
+use Test::Warn;
+
+BEGIN {
+        use_ok('C4::SIP::Sip');
+}
+
+my $date_time = C4::SIP::Sip::timestamp();
+like( $date_time, qr/^\d{8}    \d{6}$/, 'Timestamp format no param');
+
+my $t = time();
+
+$date_time = C4::SIP::Sip::timestamp($t);
+like( $date_time, qr/^\d{8}    \d{6}$/, 'Timestamp format secs');
+
+$date_time = C4::SIP::Sip::timestamp('2011-01-12');
+ok( $date_time eq '20110112    235900', 'Timestamp iso date string');
+
+my $myChecksum = C4::SIP::Sip::Checksum::checksum("12345");
+my $checker = 65281;
+my $stringChecksum = C4::SIP::Sip::Checksum::checksum("teststring");
+my $stringChecker = 64425;
+
+is( $myChecksum, $checker, "Checksum: $myChecksum matches expected output");
+is( $stringChecksum, $stringChecker, "Checksum: $stringChecksum matches expected output");
+
+my $testdata = "abcdAZ";
+my $something = C4::SIP::Sip::Checksum::checksum($testdata);
+
+$something =  sprintf("%4X", $something);
+ok( C4::SIP::Sip::Checksum::verify_cksum($testdata.$something), "Checksum: $something is valid.");
+
+my $invalidTest;
+warning_is { $invalidTest = C4::SIP::Sip::Checksum::verify_cksum("1234567") }
+            'verify_cksum: no sum detected',
+            'verify_cksum prints the expected warning for an invalid checksum';
+is($invalidTest, 0, "Checksum: 1234567 is invalid as expected");
+
+1;

diff --git a/t/SIP_Sip.t b/t/SIP_Sip.t
deleted file mode 100755 (executable)
index f670277..0000000
--- a/t/SIP_Sip.t
+++ /dev/null
@@ -1,58 +0,0 @@
-#!/usr/bin/perl
-
-# This file is part of Koha.
-#
-# Koha is free software; you can redistribute it and/or modify it
-# under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3 of the License, or
-# (at your option) any later version.
-#
-# Koha is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Koha; if not, see <http://www.gnu.org/licenses>.
-
-use Modern::Perl;
-
-use Test::More tests => 9;
-use Test::Warn;
-
-BEGIN {
-        use_ok('C4::SIP::Sip');
-}
-
-my $date_time = C4::SIP::Sip::timestamp();
-like( $date_time, qr/^\d{8}    \d{6}$/, 'Timestamp format no param');
-
-my $t = time();
-
-$date_time = C4::SIP::Sip::timestamp($t);
-like( $date_time, qr/^\d{8}    \d{6}$/, 'Timestamp format secs');
-
-$date_time = C4::SIP::Sip::timestamp('2011-01-12');
-ok( $date_time eq '20110112    235900', 'Timestamp iso date string');
-
-my $myChecksum = C4::SIP::Sip::Checksum::checksum("12345");
-my $checker = 65281;
-my $stringChecksum = C4::SIP::Sip::Checksum::checksum("teststring");
-my $stringChecker = 64425;
-
-is( $myChecksum, $checker, "Checksum: $myChecksum matches expected output");
-is( $stringChecksum, $stringChecker, "Checksum: $stringChecksum matches expected output");
-
-my $testdata = "abcdAZ";
-my $something = C4::SIP::Sip::Checksum::checksum($testdata);
-
-$something =  sprintf("%4X", $something);
-ok( C4::SIP::Sip::Checksum::verify_cksum($testdata.$something), "Checksum: $something is valid.");
-
-my $invalidTest;
-warning_is { $invalidTest = C4::SIP::Sip::Checksum::verify_cksum("1234567") }
-            'verify_cksum: no sum detected',
-            'verify_cksum prints the expected warning for an invalid checksum';
-is($invalidTest, 0, "Checksum: 1234567 is invalid as expected");
-
-1;

diff --git a/t/db_dependent/SIP/SIPServer.t b/t/db_dependent/SIP/SIPServer.t
new file mode 100755 (executable)
index 0000000..588dde1
--- /dev/null
+++ b/t/db_dependent/SIP/SIPServer.t
@@ -0,0 +1,78 @@
+#!/usr/bin/perl
+
+# This test is db dependent: SIPServer needs MsgType which needs Auth.
+# And Auth needs config vars and prefences in its BEGIN block.
+
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# Koha is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Koha; if not, see <http://www.gnu.org/licenses>.
+
+use Modern::Perl;
+
+use Test::More tests => 1;
+use Test::Warn;
+
+my ( $mockConfig, $mockPrefork );
+
+BEGIN {
+    # In order to test SIPServer::get_timeout, we need to mock
+    # Configuration->new and PreFork->run.
+    use Test::MockModule;
+    use C4::SIP::Sip::Configuration;
+    $mockConfig = Test::MockModule->new( 'C4::SIP::Sip::Configuration' );
+    $mockConfig->mock( 'new', sub { return {}; } );
+    use Net::Server::PreFork;
+    $mockPrefork = Test::MockModule->new( 'Net::Server::PreFork' );
+    $mockPrefork->mock( 'run', sub {} );
+}
+
+use C4::SIP::SIPServer;
+
+# Start testing !
+# TODO We should include more tests here.
+
+subtest 'Get_timeout' => sub {
+    plan tests => 11;
+
+    my $server = { policy => { timeout => 1 },
+                   config => { timeout => 2 },
+                   service => {
+                       timeout => 3,
+                       client_timeout => 4,
+                   },
+    };
+
+    is( C4::SIP::SIPServer::get_timeout(), 30, "Default fallback" );
+    is( C4::SIP::SIPServer::get_timeout( undef, { fallback => 25 } ), 25, "Fallback parameter" );
+    is( C4::SIP::SIPServer::get_timeout( $server, { transport => 1 } ), 3, "Transport value" );
+    is( C4::SIP::SIPServer::get_timeout( $server, { client => 1 } ), 4, "Client value" );
+    is( C4::SIP::SIPServer::get_timeout( $server, { policy => 1 } ), '001', "Policy value" );
+
+    delete $server->{policy}->{timeout};
+    is( C4::SIP::SIPServer::get_timeout( $server, { policy => 1 } ), '000', "No policy" );
+
+    $server->{service}->{client_timeout} = '0';
+    is( C4::SIP::SIPServer::get_timeout( $server, { client => 1 } ), 0, "Client zero" );
+    $server->{service}->{client_timeout} = 'no';
+    is( C4::SIP::SIPServer::get_timeout( $server, { client => 1 } ), 0, "Client no" );
+    delete $server->{service}->{client_timeout};
+    is( C4::SIP::SIPServer::get_timeout( $server, { client => 1 } ), 3, "Fallback to service" );
+
+    delete $server->{service}->{timeout};
+    is( C4::SIP::SIPServer::get_timeout( $server, { transport => 1 } ), 2, "Back to old config" );
+    delete $server->{config}->{timeout};
+    is( C4::SIP::SIPServer::get_timeout( $server, { transport => 1 } ), 30, "Fallback again" );
+};
+
+1;