Bugzilla – Attachment 53358 Details for
Bug 15006
Need to distinguish client timeout from login timeout
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[SIGNED-OFF] Bug 15006: Centralize timeout logic and allow zero client timeout
Bug-15006-Centralize-timeout-logic-and-allow-zero-.patch (text/plain), 10.44 KB, created by
Srdjan Jankovic
on 2016-07-13 00:57:43 UTC
(
hide
)
Description:
[SIGNED-OFF] Bug 15006: Centralize timeout logic and allow zero client timeout
Filename:
MIME Type:
Creator:
Srdjan Jankovic
Created:
2016-07-13 00:57:43 UTC
Size:
10.44 KB
patch
obsolete
>From 8b7b50eb77e14fcd8e986efdb6d26286b61d16ee Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >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 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@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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=53358">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
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