Merge pull request cyrusimap#5123 from elliefm/v311/master-cleanup-after-crashed-service

clean up proc entries for crashed services

Author: elliefm

cassandane/Cassandane/Cyrus/Info.pm

@@ -43,6 +43,7 @@ use warnings;
 use Cwd qw(realpath);
 use Data::Dumper;
 use Date::Format qw(time2str);
+use Time::HiRes qw(usleep);
 
 use lib '.';
 use base qw(Cassandane::Cyrus::TestCase);
@@ -296,6 +297,63 @@ sub test_proc_services
     }
 }
 
+sub test_proc_crashed_services
+{
+    my ($self) = @_;
+
+    # no clients => no service daemons => no processes
+    my @output = $self->{instance}->run_cyr_info('proc');
+    $self->assert_num_equals(0, scalar @output);
+
+    # master spawns service processes when clients connect to them
+    my $imap_svc = $self->{instance}->get_service('imap');
+    my @clients;
+    foreach (1..5) {
+        # five concurrent connections for a single user is normal,
+        # e.g. thunderbird does this
+        my $store = $imap_svc->create_store(username => 'cassandane');
+        my $imaptalk = $store->get_client();
+        push @clients, $imaptalk if $imaptalk;
+    }
+
+    # better have got some clients from that!
+    $self->assert_num_gte(1, scalar @clients);
+
+    # five clients => five service daemons => five processes
+    @output = $self->{instance}->run_cyr_info('proc');
+    $self->assert_num_equals(scalar @clients, scalar @output);
+
+    my @pids = sort map { (split /\s+/, $_, 2)[0] } @output;
+    $self->assert_num_equals(scalar @clients, scalar @pids);
+
+    # crash service processes one at a time, expect proc count to decrease
+    while (scalar @pids) {
+        my $pid = shift @pids;
+        kill 'SEGV', $pid;
+        usleep 250_000;
+
+        my @cores = $self->{instance}->find_cores();
+        if (@cores) {
+            # if we dumped core, there'd better only be one core file
+            $self->assert_num_equals(1, scalar @cores);
+
+            # don't barf on it existing during shutdown
+            unlink $cores[0];
+        }
+
+        @output = $self->{instance}->run_cyr_info('proc');
+        $self->assert_num_equals(scalar @pids, scalar @output);
+    }
+
+    # prevent a lot of "Connection closed by other end" noise by claiming
+    # and discarding the client's socket before its DESTROY is called
+    while (scalar @clients) {
+        my $old = shift @clients;
+
+        $old->release_socket(1);
+    }
+}
+
 sub test_proc_starts
     :NoStartInstances :needs_component_idled
 {

lib/proc.c

@@ -225,6 +225,16 @@ EXPORTED void proc_cleanup(struct proc_handle **handlep)
     }
 }
 
+/* used by master to remove proc files after service processes crash */
+EXPORTED void proc_force_cleanup(pid_t pid)
+{
+    char *fname = proc_getpath(pid, /*isnew*/0);
+
+    if (fname)
+        xunlink(fname);
+    free(fname);
+}
+
 static int proc_foreach_helper(pid_t pid, procdata_t *func, void *rock)
 {
     int r = 0;

lib/proc.h

@@ -52,6 +52,7 @@ extern int proc_register(struct proc_handle **handlep,
                          const char *mailbox,
                          const char *cmd);
 extern void proc_cleanup(struct proc_handle **handlep);
+extern void proc_force_cleanup(pid_t pid);
 
 typedef int procdata_t(pid_t pid,
                        const char *servicename, const char *clienthost,

master/master.c

@@ -1525,7 +1525,16 @@ static void reap_child(void)
                 }
             }
 
-            proc_cleanup(&c->proc_handle);
+            if (s && !in_shutdown && failed) {
+                /* we don't have a proc_handle for service processes because
+                 * they manage it themselves, but if one crashed it won't have
+                 * cleaned it up
+                 */
+                proc_force_cleanup(pid);
+            }
+            else {
+                proc_cleanup(&c->proc_handle);
+            }
             centry_set_state(c, SERVICE_STATE_DEAD);
         } else {
             /* Are we multithreaded now? we don't know this child */