diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index e770bce0e..43a8f73ae 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -125,10 +125,10 @@ sub reported { # The one impure sub. Everything above only does arithmetic; this actually forks. # Fork a child and keep the pacing straight while doing it. Takes the child's body as a -# block, then `state`, `pid` and `now`, and hands back the state and the new pid: +# block, then `state` and `pid` -- REFERENCES to the caller's own variables -- and `now`: # -# ($state, $pid) = xCAT::RespawnUtils::supervise { ...child... } -# state => $state, pid => $pid, now => time(); +# xCAT::RespawnUtils::supervise { ...child... } +# state => \$state, pid => \$pid, now => time(); # # The (&@) prototype is what allows the leading block. It needs this module loaded with # `use`, not `require`: under `require` the sub is unknown when the call is compiled, the @@ -136,36 +136,49 @@ sub reported { # # Two orderings in here are easy to get wrong and are the reason this is not left to callers. # The attempt is recorded before the fork, because the child can die and be reaped before -# fork() returns to us. And SIGCHLD is blocked across the fork and the assignment, because a -# reaper that matches on the pid would otherwise compare against a stale one, miss the death, -# and leave the caller believing a dead child is still alive. +# fork() returns to us. And the pid and the state are installed in the caller's variables +# while SIGCHLD is still blocked -- which is why they are passed by reference rather than +# handed back as a return value. The reaper matches the dead child against that pid and folds +# the death into that state; had the caller assigned them from a return value, the assignment +# would land after the signal was let back in, so a child dying in the gap would be compared +# against a pid still holding 0, missed, and the caller would then write a dead pid back over +# the reaper's work -- believing a dead child alive, and never respawning it. # # The block is only ever entered in the child and is not expected to return; if it does, the # child exits quietly rather than falling back into the parent's code. Passing a live `pid` -# is a no-op, so a caller that forgets to check is not punished with a second child. +# is a no-op, so a caller that forgets to check is not punished with a second child. The new +# pid is also returned, for a caller that wants it inline. sub supervise (&@) { my ($child, %arg) = @_; - my ($state, $pid, $now) = @arg{qw(state pid now)}; + my ($stateref, $pidref, $now) = @arg{qw(state pid now)}; - return ($state, $pid) if $pid; # already running; nothing to do + return $$pidref if $$pidref; # already running; nothing to do require POSIX; require xCAT::Utils; - $state = forked($state, $now); - my $mask = POSIX::SigSet->new(POSIX::SIGCHLD()); POSIX::sigprocmask(POSIX::SIG_BLOCK(), $mask); - $pid = xCAT::Utils->xfork; - POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); - return (exited($state, $now), 0) unless defined $pid; # could not fork: back off + $$stateref = forked($$stateref, $now); + my $pid = xCAT::Utils->xfork; - unless ($pid) { + unless (defined $pid) { # could not fork: count it and back off + $$stateref = exited($$stateref, $now); + $$pidref = 0; + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return 0; + } + + unless ($pid) { # child: it must not go on to serve with SIGCHLD blocked + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); $child->(); POSIX::_exit(0); } - return ($state, $pid); + + $$pidref = $pid; # in place before the reaper can run, or it matches a stale pid + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return $pid; } 1; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 74c0f6293..0ca56274a 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1203,15 +1203,17 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -# supervise() records the attempt and blocks SIGCHLD across the fork; this monitor's uptime -# counts towards the pacing too, so an unrelated death much later is retried promptly. -($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { +# supervise() records the attempt, blocks SIGCHLD across the fork, and installs $pid_MON and +# $mon_respawn itself while it is still blocked -- which is why they are passed by reference; +# see the sub. This monitor's uptime counts towards the pacing too, so an unrelated death much +# later is retried promptly. +xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($udpctl); $udpctl = 0; do_installm_service; xexit(0); -} state => $mon_respawn, pid => $pid_MON, now => time(); +} state => \$mon_respawn, pid => \$pid_MON, now => time(); unless ($pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); @@ -1494,7 +1496,7 @@ until ($quit) { . " still retrying xcatiport $sport every $mon_respawn->{max_interval} seconds"); $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } - ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($listener); @@ -1507,7 +1509,7 @@ until ($quit) { close($chwritepipe); do_installm_service; xexit(0); - } state => $mon_respawn, pid => $pid_MON, now => time(); + } state => \$mon_respawn, pid => \$pid_MON, now => time(); if ($pid_MON) { xCAT::MsgUtils->trace(0, "I", diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 8f1cc0b11..fe7a36751 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -223,10 +223,10 @@ subtest 'the pid and the pacing are in place before SIGCHLD is let back in' => s } select( undef, undef, undef, 0.2 ); # it is gone, and its SIGCHLD is pending, not delivered - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { sleep 3600; # a monitor that stays up; this one is about the parent } - state => $pace, pid => $mon_pid, now => time(); + state => \$pace, pid => \$mon_pid, now => time(); push @spawned, $mon_pid if $mon_pid; ok( $ran, 'the pending SIGCHLD was delivered while supervise() was still running' ); @@ -291,7 +291,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { # Driven through supervise() -- the same call xcatd makes -- so this exercises the # real fork-and-account sequence rather than a copy of it here. - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { close($holder) if $holder; # never hold the port from inside a child my $sock = IO::Socket::INET->new( LocalAddr => '127.0.0.1', @@ -303,7 +303,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does sleep 3600; # bound the port and serve } - state => $pace, pid => $mon_pid, now => $now; + state => \$pace, pid => \$mon_pid, now => $now; die "supervise did not fork" unless $mon_pid; $mon_forked_at = $now;