Bug #73711 for Server-Starter: Failing test / install

Dne Út 03.led.2012 16:03:11, landsidel.allen@gmail.com napsal(a): Show quoted text
I experience similar problem with t/07-envdir.t. Removing the sleep's around the kill call make the failures fairly reproducible. Attached two patches fixes the problem. The 0001-Synchronize-to-PID-in-t-07-envdir.t.patch patch fixes the t/07-envdir.t test, the 0002-Fix-loading-envdir.patch fixes loading the environment directory. -- Petr

From 9fc8c81a018ba6cb5f3f0ad3fbc836e301468978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Mon, 16 Jun 2014 15:20:43 +0200 Subject: [PATCH 1/2] Synchronize to PID in t/07-envdir.t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The t/07-envdir.t used various sleeps to deal with races. This broke obviously when the timing was abnormal like on have loaded machine. This patch synchronizes on the status file by checking only the new worker is running. Similar to CPAN RT#73711. Signed-off-by: Petr Písař <ppisar@redhat.com> --- t/07-envdir-print.pl | 3 ++- t/07-envdir.t | 29 ++++++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/t/07-envdir-print.pl b/t/07-envdir-print.pl index 22cb01a..fb14225 100755 --- a/t/07-envdir-print.pl +++ b/t/07-envdir-print.pl @@ -11,6 +11,7 @@ use Server::Starter qw(server_ports); $SIG{TERM} = $SIG{USR1} = sub { exit 0; }; +$ENV{PID} = $$; my $listener = IO::Socket::INET->new( Proto => 'tcp', @@ -21,7 +22,7 @@ $listener->fdopen((values %{server_ports()})[0], 'w') while (1) { if (my $conn = $listener->accept) { my $s = ""; - for my $envkey (keys %ENV) { + for my $envkey (sort keys %ENV) { $s .= $envkey . "=" . $ENV{$envkey} . "\n"; } $conn->syswrite($s); diff --git a/t/07-envdir.t b/t/07-envdir.t index 8bf2352..2050706 100644 --- a/t/07-envdir.t +++ b/t/07-envdir.t @@ -46,13 +46,36 @@ test_tcp( $buf; }; my $restart = sub { - sleep 1; + sub getstatus { + my ($file, $value); + open($file, '<', "$tempdir/status") or return ''; + do { local $/ = undef; $value = <$file>; }; + close $file; + $value // ''; + } + sub getsinglegeneration { + my $status; + do { + sleep 1 if defined $status; + $status = getstatus; + } until ($status =~ /\A\d+:\d+\n\z/); + chomp $status; + $status; + } + my $previous_generation = getsinglegeneration; kill "HUP", $server_pid; - sleep 2; + my $current_generation; + while (($current_generation = getsinglegeneration) eq + $previous_generation) { + sleep 1; + } + diag "Server changed from <$previous_generation> ", + "to <$current_generation>\n"; }; # Initial worker does not read envdir my $buf = $fetch_env->(); - unlike($buf, qr/^FOO=foo-value1$/m, 'changed env'); + unlike($buf, qr/^FOO=foo-value1$/m, + 'environment not read for the first time'); # rewrite envdir open my $envfh, ">", "$tempdir/env/FOO" or die $!; print $envfh "foo-value2"; -- 1.9.3

From 6d965848ff8905f82da0f1ac142000b12a05905e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Mon, 16 Jun 2014 17:25:08 +0200 Subject: [PATCH 2/2] Fix loading envdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The envdir was scanned each second regardless of signal received. This caused random failures of the 'removed env' t/07-envdir.t test. This patch fixes it by loading the environment only just before intended restart. It also documents this start_server() option. Signed-off-by: Petr Písař <ppisar@redhat.com> --- lib/Server/Starter.pm | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/lib/Server/Starter.pm b/lib/Server/Starter.pm index 47bc18a..c171307 100644 --- a/lib/Server/Starter.pm +++ b/lib/Server/Starter.pm @@ -179,15 +179,13 @@ sub start_server { # the main loop my $term_signal = 0; - $current_worker = _start_worker($opts); + my %loaded_env; + $current_worker = _start_worker($opts, \%loaded_env); $update_status->(); my $auto_restart_interval = 0; my $last_restart_time = time(); my $restart_flag = 0; while (1) { - my %loaded_env = _reload_env(); - my @loaded_env_keys = keys %loaded_env; - local @ENV{@loaded_env_keys} = map { $loaded_env{$_} } (@loaded_env_keys); if ($ENV{ENABLE_AUTO_RESTART}) { # restart workers periodically $auto_restart_interval = $ENV{AUTO_RESTART_INTERVAL} ||= 360; @@ -201,7 +199,7 @@ sub start_server { last if ($died_worker <= 0); if ($died_worker == $current_worker) { print STDERR "worker $died_worker died unexpectedly with status:$status, restarting\n"; - $current_worker = _start_worker($opts); + $current_worker = _start_worker($opts, \%loaded_env); $last_restart_time = time(); } else { print STDERR "old worker $died_worker died, status:$status\n"; @@ -232,9 +230,10 @@ sub start_server { } } if ($restart_flag > 1 || ($restart_flag > 0 && $num_old_workers == 0)) { + %loaded_env = _reload_env(); print STDERR "spawning a new worker (num_old_workers=$num_old_workers)\n"; $old_workers{$current_worker} = $ENV{SERVER_STARTER_GENERATION}; - $current_worker = _start_worker($opts); + $current_worker = _start_worker($opts, \%loaded_env); $last_restart_time = time(); $restart_flag = 0; $update_status->(); @@ -342,7 +341,7 @@ sub _reload_env { } sub _start_worker { - my $opts = shift; + my ($opts, $loaded_env) = @_; my $pid; while (1) { $ENV{SERVER_STARTER_GENERATION}++; @@ -350,6 +349,8 @@ sub _start_worker { die "fork(2) failed:$!" unless defined $pid; if ($pid == 0) { + my @loaded_env_keys = keys %$loaded_env; + @ENV{@loaded_env_keys} = map { $loaded_env->{$_} } (@loaded_env_keys); my @args = @{$opts->{exec}}; # child process if (defined $opts->{dir}) { @@ -418,9 +419,21 @@ A Net::Server personality that can be run under L<Server::Starter> exists under Returns zero or more file descriptors on which the server program should call accept(2) in a hashref. Each element of the hashref is: (host:port|port|path_of_unix_socket) => file_descriptor. -=item start_server +=item start_server(%options) -Starts the superdaemon. Used by the C<start_server> script. +Starts the superdaemon. Used by the C<start_server> script. Options are: + +=over 4 + +=item envdir + +Defines a directory whose content is added into server's environment. Each file name specifies an environment variable, the file's content specifies a value of the environment variable. Files with names starting with a dot are ignored. + +The environment directory is scanned and read only on time-based autorestart or SIGHUP signal. The first server instances have default environment. To remove an enviroment variable added before, remove the file and send the signal. + +This option is not mandatory. + +=back =back -- 1.9.3

Dne Út 03.led.2012 16:03:11, landsidel.allen@gmail.com napsal(a): Show quoted text
There are similar problems in t/06-autorestart.t. Attached patch should fix them. -- Petr

From b2cee396fea96266a95a829b94cdf759d0eae76d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Thu, 10 Jul 2014 15:37:47 +0200 Subject: [PATCH] Synchronize to PID in t/06-autorestart.t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were races between various sleeps and 4s auto_restart_interval. This patch replaces the sleeps by waiting for status with a single PID entry. Similar to CPAN RT#73711. Signed-off-by: Petr Písař <ppisar@redhat.com> --- t/06-autorestart.t | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/t/06-autorestart.t b/t/06-autorestart.t index bab9241..9401500 100644 --- a/t/06-autorestart.t +++ b/t/06-autorestart.t @@ -3,7 +3,7 @@ use warnings; use File::Temp (); use Test::TCP; -use Test::More tests => 28; +use Test::More tests => 26; use Server::Starter qw(start_server); @@ -46,13 +46,19 @@ for my $signal_on_hup ('TERM', 'USR1') { $buf =~ /^(\d+):/; my $worker_pid = $1; # switch to next gen - sleep 2; - my $status = get_status(); - like(get_status(), qr/^1:\d+\n$/s, 'status before auto-restart'); - sleep 5; - like(get_status(), qr/^1:\d+\n2:\d+$/s, 'status during transient state'); - sleep 4; - like(get_status(), qr/^2:\d+\n$/s, 'status after auto-restart'); + my $previous_generation = get_single_generation(); + like($previous_generation, qr/^\d+:\d+\n$/s, + 'status before auto-restart'); + my $current_generation; + while (($current_generation = get_single_generation()) eq + $previous_generation) { + sleep 1; + } + diag "Server changed from <$previous_generation> ", + "to <$current_generation>\n"; + + like($current_generation, qr/^\d+:\d+\n$/s, + 'status after auto-restart'); is( do { open my $fh, '<', "$tempdir/signame" @@ -78,7 +84,20 @@ for my $signal_on_hup ('TERM', 'USR1') { } sub get_status { + while (! -e "$tempdir/status") { + sleep 1; + } open my $fh, '<', "$tempdir/status" or die "failed to open file:$tempdir/status:$!"; do { undef $/; <$fh> }; } + +sub get_single_generation { + my $status; + do { + sleep 1 if defined $status; + $status = get_status; + } until ($status =~ /\A\d+:\d+\n\z/); + chomp $status; + $status; +} -- 1.9.3

Dne Út 03.led.2012 16:03:11, landsidel.allen@gmail.com napsal(a): Show quoted text
There is similar bug in t/05-killolddelay.t. Attached patch fixes it.

From 046fb1328e76851a1398a624b77e746c37c62a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Thu, 21 Aug 2014 14:17:25 +0200 Subject: [PATCH] Synchronize to PID in t/05-killolddelay.t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were races between various sleeps and the restarting server. This patch fixes it by waiting for complete generation change in the status file. Similar to CPAN RT#73711. Signed-off-by: Petr Písař <ppisar@redhat.com> --- t/05-killolddelay.t | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/t/05-killolddelay.t b/t/05-killolddelay.t index 19856af..457bdd9 100644 --- a/t/05-killolddelay.t +++ b/t/05-killolddelay.t @@ -3,7 +3,7 @@ use warnings; use File::Temp (); use Test::TCP; -use Test::More tests => 28; +use Test::More tests => 26; use Server::Starter qw(start_server); @@ -45,17 +45,18 @@ for my $signal_on_hup ('TERM', 'USR1') { $buf =~ /^(\d+):/; my $worker_pid = $1; # switch to next gen - sleep 2; - my $status = get_status(); - like(get_status(), qr/^1:\d+\n$/s, 'status before restart'); + my $previous_generation = get_single_generation(); + like($previous_generation, qr/^1:\d+\n$/s, 'status before restart'); kill 'HUP', $server_pid; - sleep 4; - like(get_status(), qr/^1:\d+\n2:\d+$/s, 'status during restart'); - # Child process has finished in 2 seconds but the parent - # checks and calls waitpid every second, so wait for an - # additional 1 second. - sleep 3; - like(get_status(), qr/^2:\d+\n$/s, 'status after restart'); + my $current_generation; + while (($current_generation = get_single_generation()) eq + $previous_generation) { + sleep 1; + } + diag "Server changed from <$previous_generation> ", + "to <$current_generation>\n"; + + like($current_generation, qr/^2:\d+\n$/s, 'status after restart'); is( do { open my $fh, '<', "$tempdir/signame" @@ -81,7 +82,20 @@ for my $signal_on_hup ('TERM', 'USR1') { } sub get_status { + while (! -e "$tempdir/status") { + sleep 1; + } open my $fh, '<', "$tempdir/status" or die "failed to open file:$tempdir/status:$!"; do { undef $/; <$fh> }; } + +sub get_single_generation { + my $status; + do { + sleep 1 if defined $status; + $status = get_status; + } until ($status =~ /\A\d+:\d+\n\z/); + chomp $status; + $status; +} -- 1.9.3