From eb8666357ce72e910569843f8acce674a332f4b0 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 6 Jul 2016 15:14:45 +0100 Subject: [PATCH] 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 Signed-off-by: Marcel de Rooy 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 --- C4/SIP/SIPServer.pm | 55 +++++++++++++++++++++--- C4/SIP/Sip/MsgType.pm | 7 +-- t/{SIP_Sip.t => SIP/Sip.t} | 0 t/db_dependent/SIP/SIPServer.t | 78 ++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 11 deletions(-) rename t/{SIP_Sip.t => SIP/Sip.t} (100%) create mode 100755 t/db_dependent/SIP/SIPServer.t diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index 0a76ca8d37..ea7bf7b472 100755 --- 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 787449bcfa..4e406e20af 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 = $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 similarity index 100% rename from t/SIP_Sip.t rename to t/SIP/Sip.t diff --git a/t/db_dependent/SIP/SIPServer.t b/t/db_dependent/SIP/SIPServer.t new file mode 100755 index 0000000000..588dde154c --- /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 . + +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; -- 2.39.5