[Koha-patches] [PATCH] Bug 15006: Centralize timeout logic and allow zero client timeout
Srdjan
srdjan at catalyst.net.nz
Wed Jul 13 02:57:46 CEST 2016
From: Marcel de Rooy <m.de.rooy at rijksmuseum.nl>
Moving timeout logic to one routine in Sip.pm (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/SIP_Sip.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 at catalyst.net.nz>
---
C4/SIP/SIPServer.pm | 12 ++++---
C4/SIP/Sip.pm | 46 +++++++++++++++++++++++++-
C4/SIP/Sip/MsgType.pm | 7 +---
t/SIP_Sip.t | 90 +++++++++++++++++++++++++++++++++++++--------------
4 files changed, 119 insertions(+), 36 deletions(-)
diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm
index 0a76ca8..e3cab23 100755
--- a/C4/SIP/SIPServer.pm
+++ b/C4/SIP/SIPServer.pm
@@ -15,6 +15,7 @@ use C4::SIP::Sip::Constants qw(:all);
use C4::SIP::Sip::Configuration;
use C4::SIP::Sip::Checksum qw(checksum verify_cksum);
use C4::SIP::Sip::MsgType qw( handle login_core );
+use C4::SIP::Sip qw( get_timeout );
use base qw(Net::Server::PreFork);
@@ -135,8 +136,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 = get_timeout( $self, { transport => 1 } );
+ syslog('LOG_DEBUG', "raw_transport: timeout is $timeout");
+ alarm $timeout;
while (!$self->{account}) {
$input = read_request();
if (!$input) {
@@ -193,8 +195,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 = get_timeout( $self, { 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 +258,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 = get_timeout( $self, { client => 1 } );
# The spec says the first message will be:
# SIP v1: SC_STATUS
diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm
index 52eda1f..ec549e6 100644
--- a/C4/SIP/Sip.pm
+++ b/C4/SIP/Sip.pm
@@ -22,17 +22,19 @@ BEGIN {
@ISA = qw(Exporter);
@EXPORT_OK = qw(y_or_n timestamp add_field maybe_add add_count
- denied sipbool boolspace write_msg
+ denied sipbool boolspace write_msg get_timeout
$error_detection $protocol_version $field_delimiter
$last_response);
%EXPORT_TAGS = (
all => [qw(y_or_n timestamp add_field maybe_add
add_count denied sipbool boolspace write_msg
+ get_timeout
$error_detection $protocol_version
$field_delimiter $last_response)]);
}
+
our $error_detection = 0;
our $protocol_version = 1;
our $field_delimiter = '|'; # Protocol Default
@@ -197,4 +199,46 @@ sub write_msg {
$last_response = $msg;
}
+# get_timeout( $server, { $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;
diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
index 787449b..8a87f31 100644
--- 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 = get_timeout( $server, { 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
index f670277..2486ddc 100755
--- a/t/SIP_Sip.t
+++ b/t/SIP_Sip.t
@@ -17,42 +17,84 @@
use Modern::Perl;
-use Test::More tests => 9;
+use Test::More tests => 3;
use Test::Warn;
BEGIN {
- use_ok('C4::SIP::Sip');
+ use_ok('C4::SIP::Sip', qw|get_timeout| );
}
-my $date_time = C4::SIP::Sip::timestamp();
-like( $date_time, qr/^\d{8} \d{6}$/, 'Timestamp format no param');
+subtest 'Timestamp' => sub {
+ plan tests => 8;
+ test_timestamp();
+};
+subtest 'Get_timeout' => sub {
+ plan tests => 11;
+ test_get_timeout();
+};
-my $t = time();
+sub test_timestamp {
+ my $date_time = C4::SIP::Sip::timestamp();
+ like( $date_time, qr/^\d{8} \d{6}$/, 'Timestamp format no param');
-$date_time = C4::SIP::Sip::timestamp($t);
-like( $date_time, qr/^\d{8} \d{6}$/, 'Timestamp format secs');
+ my $t = time();
-$date_time = C4::SIP::Sip::timestamp('2011-01-12');
-ok( $date_time eq '20110112 235900', 'Timestamp iso date string');
+ $date_time = C4::SIP::Sip::timestamp($t);
+ like( $date_time, qr/^\d{8} \d{6}$/, 'Timestamp format secs');
-my $myChecksum = C4::SIP::Sip::Checksum::checksum("12345");
-my $checker = 65281;
-my $stringChecksum = C4::SIP::Sip::Checksum::checksum("teststring");
-my $stringChecker = 64425;
+ $date_time = C4::SIP::Sip::timestamp('2011-01-12');
+ ok( $date_time eq '20110112 235900', 'Timestamp iso date string');
-is( $myChecksum, $checker, "Checksum: $myChecksum matches expected output");
-is( $stringChecksum, $stringChecker, "Checksum: $stringChecksum matches expected output");
+ my $myChecksum = C4::SIP::Sip::Checksum::checksum("12345");
+ my $checker = 65281;
+ my $stringChecksum = C4::SIP::Sip::Checksum::checksum("teststring");
+ my $stringChecker = 64425;
-my $testdata = "abcdAZ";
-my $something = C4::SIP::Sip::Checksum::checksum($testdata);
+ is( $myChecksum, $checker, "Checksum: $myChecksum matches expected output");
+ is( $stringChecksum, $stringChecker, "Checksum: $stringChecksum matches expected output");
-$something = sprintf("%4X", $something);
-ok( C4::SIP::Sip::Checksum::verify_cksum($testdata.$something), "Checksum: $something is valid.");
+ my $testdata = "abcdAZ";
+ my $something = C4::SIP::Sip::Checksum::checksum($testdata);
-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");
+ $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");
+}
+
+sub test_get_timeout {
+ my $server = { policy => { timeout => 1 },
+ config => { timeout => 2 },
+ service => {
+ timeout => 3,
+ client_timeout => 4,
+ },
+ };
+
+ is( get_timeout(), 30, "Default fallback" );
+ is( get_timeout( undef, { fallback => 25 } ), 25, "Fallback parameter" );
+ is( get_timeout( $server, { transport => 1 } ), 3, "Transport value" );
+ is( get_timeout( $server, { client => 1 } ), 4, "Client value" );
+ is( get_timeout( $server, { policy => 1 } ), '001', "Policy value" );
+
+ delete $server->{policy}->{timeout};
+ is( get_timeout( $server, { policy => 1 } ), '000', "No policy" );
+
+ $server->{service}->{client_timeout} = '0';
+ is( get_timeout( $server, { client => 1 } ), 0, "Client zero" );
+ $server->{service}->{client_timeout} = 'no';
+ is( get_timeout( $server, { client => 1 } ), 0, "Client no" );
+ delete $server->{service}->{client_timeout};
+ is( get_timeout( $server, { client => 1 } ), 3, "Fallback to service" );
+
+ delete $server->{service}->{timeout};
+ is( get_timeout( $server, { transport => 1 } ), 2, "Back to old config" );
+ delete $server->{config}->{timeout};
+ is( get_timeout( $server, { transport => 1 } ), 30, "Fallback again" );
+}
1;
--
2.7.4
More information about the Koha-patches
mailing list