Attachment 53014 Details for Bug 15006 – Bug 15006: Centralize timeout logic and allow zero client timeout

[patch] Bug 15006: Centralize timeout logic and allow zero client timeout

Bug-15006-Centralize-timeout-logic-and-allow-zero-.patch (text/plain), 11.25 KB, created by Marcel de Rooy on 2016-07-01 10:16:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2016-07-01 10:16:00 UTC

Size: 11.25 KB

patch

obsolete

>From f84869b1abf29b2314e7d09a721a2512ad83702d Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Fri, 17 Jun 2016 13:42:53 +0200
>Subject: [PATCH] Bug 15006: Centralize timeout logic and allow zero client
> timeout
>Content-Type: text/plain; charset=utf-8
>
>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: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Amended after feedback of Colin. Added a test.
>---
> C4/SIP/SIPServer.pm   |    9 +++--
> C4/SIP/Sip.pm         |   47 ++++++++++++++++++++++++--
> C4/SIP/Sip/MsgType.pm |    7 +---
> etc/SIPconfig.xml     |    4 +--
> t/SIP_Sip.t           |   90 ++++++++++++++++++++++++++++++++++++-------------
> 5 files changed, 120 insertions(+), 37 deletions(-)
>
>diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm
>index a972182..7ef2afc 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
>@@ -341,5 +342,7 @@ sub read_request {
>       syslog( 'LOG_INFO', "INPUT MSG: '$buffer'" );
>       return $buffer;
> }
>+
> 1;
>+
> __END__
>diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm
>index e5bdfa1..e83e887 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,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/etc/SIPconfig.xml b/etc/SIPconfig.xml
>index 8bd69f1..7a7a240 100644
>--- a/etc/SIPconfig.xml
>+++ b/etc/SIPconfig.xml
>@@ -28,9 +28,9 @@
>       protocol="SIP/2.00"
>       timeout="60" />
> 
>diff --git a/t/SIP_Sip.t b/t/SIP_Sip.t
>index f670277..0c50609 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;
>-- 
>1.7.10.4

Actions: View | Diff | Splinter Review

Attachments on bug 15006: 45039 | 45583 | 45594 | 52477 | 52478 | 52479 | 52480 | 52784 | 52785 | 52786 | 52787 | 52849 | 52850 | 52851 | 52852 | 52853 | 53009 | 53010 | 53011 | 53012 | 53013 | 53014 | 53132 | 53133 | 53134 | 53135 | 53309 | 53310 | 53311 | 53312 | 53355 | 53356 | 53357 | 53358 | 53359 | 53360 | 53363 | 53364 | 53365 | 53366 | 53367 | 53368 | 53369