Avoid giving up a lock when we are forking

case CPANEL-17403

The TaskQueue would give up the lock on the state file in order to
process as task.  Since cPanel processes most of our tasks with the
cPanel::TaskQueue::ChildProcessor task in the background this added a
write state, unlock, relock, re-read from disk cycle to every task
being processed.  Since we return right after queueprocd fork()s there
is little concern about holding the lock open.

This should also reduce the potential for unknown race conditions as we
no longer give up the lock in the middle of processing a task which had
caused unexpected results in the past.

Author: bdraco

lib/cPanel/StateFile.pm

@@ -175,6 +175,18 @@ sub import {
             return;
         }
 
+        sub _in_child {
+            my ($self) = @_;
+
+            # Ensure that the child process does not
+            # remove the lock as that is the job of
+            # the parent.
+            $self->{'lock_file'}   = undef;
+            $self->{'file_handle'} = undef;
+
+            return;
+        }
+
         sub _unlock {
             my $self       = shift;
             my $state_file = $self->{state_file};

lib/cPanel/TaskQueue.pm

@@ -589,28 +589,49 @@ END { undef %valid_processors }    # case CPANEL-10871 to avoid a SEGV during gl
         push @{ $self->{processing_list} }, $task;
         $self->_add_task_to_deferral_object( $task, $processor );
 
-        # Finished making changes, save to disk.
-        $guard->update_file();
-
-        # I don't want to stay locked while processing.
         my $pid;
         my $ex;
-        $guard->call_unlocked(
-            sub {
-                my $orig_alarm;
-                eval {
-                    local $SIG{'ALRM'} = sub { die "time out reached\n"; };
-                    $orig_alarm = alarm( $self->_timeout($processor) );
-                    $pid = $processor->process_task( $task->clone(), $self->{disk_state}->get_logger() );
-                    alarm $orig_alarm;
-                    1;
-                } or do {
-                    $ex = $@;    # save exception for later
-                    alarm $orig_alarm;
-                };
-            }
-        );
 
+        if ( $processor->isa('cPanel::TaskQueue::ChildProcessor') ) {
+
+            #
+            # Will fork() so there is no concern about the parent
+            # keeping the lock since it should return right away and
+            # will not be blocking other processes from getting a lock
+            # as soon as we return from this sub.
+            #
+            # This avoids going though a whole cycle of update_file, unlock,
+            # relock, and re-read from disk
+            #
+            # No need to set an alarm as we are going to fork() and
+            # ChildProcessor already handles timeouts
+            #
+            eval { $pid = $processor->process_task( $task->clone(), $self->{disk_state}->get_logger(), $guard ) } or do {
+                $ex = $@;
+            };
+        }
+        else {
+            # We are going to have to unlock and relock
+            # Finished making changes, save to disk.
+            $guard->update_file();
+
+            # I don't want to stay locked while processing.
+            $guard->call_unlocked(
+                sub {
+                    my $orig_alarm;
+                    eval {
+                        local $SIG{'ALRM'} = sub { die "time out reached\n"; };
+                        $orig_alarm = alarm( $self->_timeout($processor) );
+                        $pid = $processor->process_task( $task->clone(), $self->{disk_state}->get_logger() );
+                        alarm $orig_alarm;
+                        1;
+                    } or do {
+                        $ex = $@;    # save exception for later
+                        alarm $orig_alarm;
+                    };
+                }
+            );
+        }
         # Deal with a child process or remove from processing.
         if ($pid) {
             $task->set_pid($pid);

lib/cPanel/TaskQueue/ChildProcessor.pm

@@ -35,7 +35,7 @@ use cPanel::TaskQueue::Scheduler ();
     }
 
     sub process_task {
-        my ( $self, $task, $logger ) = @_;
+        my ( $self, $task, $logger, $guard ) = @_;
         my $pid = fork();
 
         $logger->throw( q{Unable to start a child process to handle the '} . $task->command() . "' task\n" )
@@ -44,6 +44,16 @@ use cPanel::TaskQueue::Scheduler ();
         # Parent returns
         return $pid if $pid;
 
+        if ( !$guard ) {
+            die __PACKAGE__ . ': expected 3 arguments to process_task($task, $logger, $guard): $guard was missing';
+        }
+
+        # Now in child
+        # Ensure the child process never unlocks
+        # the lock file as this should always be done
+        # in the parent.
+        $guard->_in_child();
+
         my $timeout = $self->get_child_timeout() || $task->child_timeout();
         my $oldalarm;
         eval {