From 1ff571f0af45944bd28e8b8446e6cf6d9ed8ea81 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 17 Jun 2016 13:42:53 +0200 Subject: [PATCH] Bug 15006: [COUNTERPATCH] Timeout logic in sip_protocol_loop Content-Type: text/plain; charset=utf-8 Instead of adding yet another client timeout, I propose to use the policy timeout that we also communicate in send_acs_status. Although I agree that we perhaps better move it from institution to account/login level, this c/should be done on another report. We may have issues with existing configurations. In order to find a compromise between bug 13432 and the originally proposed 15006 patch, this patch allows you to put a zero in the policy timeout entry in the SIPconfig. The timeout logic is moved to a Sip.pm subroutine. Will add a test :) Test plan: [1] Check timeout after login with policy/timeout > 0. [2] Check timeout after login with policy/timeout = 0. (No timeout!) [3] Check timeout after login with no policy timeout. Fallback to service. [4] Check timeout after login with no policy and service timeout. Should fallback to 30 seconds for both login process and after login. Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall --- C4/SIP/SIPServer.pm | 7 ++++--- C4/SIP/Sip.pm | 29 +++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index a972182..0ed4209 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); @@ -128,7 +129,7 @@ sub raw_transport { my $strikes = 3; local $SIG{ALRM} = sub { die "raw_transport Timed Out!\n"; }; - my $timeout = $service->{timeout} || 30; + my $timeout = get_timeout( $self, { transport => 1 } ); syslog("LOG_DEBUG", "raw_transport: timeout is %d", $timeout); while( $strikes && ( !$self->{account} || !$self->{account}->{id} )) { @@ -190,7 +191,7 @@ sub telnet_transport { my $account = undef; my $input; my $config = $self->{config}; - my $timeout = $self->{service}->{timeout} || $config->{timeout} || 30; + my $timeout = get_timeout( $self, { transport => 1 } ); syslog("LOG_DEBUG", "telnet_transport: timeout is %s", $timeout); eval { @@ -253,7 +254,7 @@ sub sip_protocol_loop { my $self = shift; my $service = $self->{service}; my $config = $self->{config}; - my $timeout = $self->{service}->{client_timeout} || $config->{client_timeout}; + 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 e5bdfa1..16f783c 100644 --- a/C4/SIP/Sip.pm +++ b/C4/SIP/Sip.pm @@ -22,14 +22,15 @@ 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 - $error_detection $protocol_version + get_timeout + $error_detection $protocol_version $field_delimiter $last_response)]); } @@ -197,4 +198,28 @@ sub write_msg { $last_response = $msg; } +# get_timeout( $server, { transport|client => 1, fallback => some_val } ); +# +# Centralizes all timeout logic from SIPServer into one routine + +sub get_timeout { + my ( $server, $params ) = @_; + + my $fallback = $params->{fallback} || 30; + if( $params->{transport} ) { + # We do not allow zero values here + return $server->{service}->{timeout} || $fallback; + } elsif( $params->{client} ) { + if( exists $server->{policy}->{timeout} ) { + # We do allow zero values here to indicate no timeout + my $val = $server->{policy}->{timeout}; + $val = 0 if ! $val > 0; # just precautious + return $val; + } + return $server->{service}->{timeout} || $fallback; + } else { + return $fallback; + } +} + 1; -- 1.7.10.4