From 93ec3272e2d78c06ecc25327986483b2ecf6635f 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 --- C4/SIP/SIPServer.pm | 19 ++++++++----------- C4/SIP/Sip.pm | 27 ++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index b04ffc7..df1ee13 100755 --- a/C4/SIP/SIPServer.pm +++ b/C4/SIP/SIPServer.pm @@ -15,7 +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( read_SIP_packet ); +use C4::SIP::Sip qw( read_SIP_packet get_timeout ); use base qw(Net::Server::PreFork); @@ -128,7 +128,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} )) { @@ -189,7 +189,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 { @@ -251,7 +251,8 @@ 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 } ); my $input; # The spec says the first message will be: @@ -268,12 +269,10 @@ sub sip_protocol_loop { # # In short, we'll take any valid message here. #my $expect = SC_STATUS; - local $SIG{ALRM} = sub { die "SIP Timed Out!\n"; } if $timeout; + local $SIG{ALRM} = sub { die "SIP Timed Out!\n"; }; my $expect = ''; while (1) { - if ($timeout) { - alarm $timeout; - } + alarm $timeout; $input = read_SIP_packet(*STDIN); unless ($input) { return; # EOF @@ -299,9 +298,7 @@ sub sip_protocol_loop { } # We successfully received and processed what we were expecting $expect = ''; - if ($timeout ) { - alarm 0; - } + alarm 0; } } diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm index a6cf97f..89ad9d1 100644 --- a/C4/SIP/Sip.pm +++ b/C4/SIP/Sip.pm @@ -23,13 +23,14 @@ BEGIN { @EXPORT_OK = qw(y_or_n timestamp add_field maybe_add add_count denied sipbool boolspace write_msg read_SIP_packet + 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 - read_SIP_packet + read_SIP_packet get_timeout $error_detection $protocol_version $field_delimiter $last_response)]); } @@ -251,4 +252,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