Francois Gouget : testbot: Better handle the database connections in case of a fork().

[Alexandre Julliard]

Module: tools
Branch: master
Commit: e50547eedef1906bd1959ec6e01b88ab16f52513
URL:    https://source.winehq.org/git/tools.git/?a=commit;h=e50547eedef1906bd1959ec6e01b88ab16f52513

Author: Francois Gouget <fgouget at codeweavers.com>
Date:   Mon Feb  5 01:31:24 2018 +0100

testbot: Better handle the database connections in case of a fork().

Make sure AutoInactiveDestroy is set. This will prevent a mere stray
fork()+exit() from breaking the parent's DBI connection.
Also stop using DBI::disconnect() as this bypasses the InactiveDestroy
setting, such that calling it in a child process will break the parent's
DBI connection. DBI connections are automatically closed in the
appropriate way their destructor when they get out of scope.
These two changes allow us to close the DBI connections in the child
process (which don't need them anyway), rather than breaking the
connections before the fork() and thus forcing the parent to reconnect.

Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard at winehq.org>

---

 testbot/bin/Engine.pl                 |  6 +++---
 testbot/lib/ObjectModel/DBIBackEnd.pm | 21 ++++++++++-----------
 testbot/lib/WineTestBot/VMs.pm        |  7 ++++---
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl
index d637564..7e278ba 100755
--- a/testbot/bin/Engine.pl
+++ b/testbot/bin/Engine.pl
@@ -309,9 +309,6 @@ sub HandleJobStatusChange($$$)
 
   if ($OldStatus eq "running" && $NewStatus ne "running")
   {
-    # Make sure the child process does not inherit the database connections
-    CloseAllDBBackEnds();
-
     my $Pid = fork;
     if (!defined $Pid)
     {
@@ -319,7 +316,10 @@ sub HandleJobStatusChange($$$)
     }
     elsif (!$Pid)
     {
+      # Clean up the child environment
+      CloseAllDBBackEnds();
       WineTestBot::Log::SetupRedirects();
+
       exec("$BinDir/${ProjectName}SendLog.pl $JobKey") or
       LogMsg "Unable to exec ${ProjectName}SendLog.pl: $!\n";
       exit(1);
diff --git a/testbot/lib/ObjectModel/DBIBackEnd.pm b/testbot/lib/ObjectModel/DBIBackEnd.pm
index 5567203..60e68d8 100644
--- a/testbot/lib/ObjectModel/DBIBackEnd.pm
+++ b/testbot/lib/ObjectModel/DBIBackEnd.pm
@@ -45,14 +45,11 @@ sub GetDb($)
 {
   my ($self) = @_;
 
-  if (defined $self->{Db} && !$self->{Db}->ping())
+  if (!$self->{Db} or !$self->{Db}->ping())
   {
-    # This connection no longer works, probably due to the database idle timeout
-    $self->{Db}->disconnect();
+    # We don't have a connection yet, or it no longer works (probably due to
+    # the database idle timeout).
     $self->{Db} = undef;
-  }
-  if (!defined $self->{Db})
-  {
     while (1)
     {
       # Protect this call so we can retry in case RaiseError is set
@@ -597,11 +594,9 @@ sub Close($)
 {
   my ($self) = @_;
 
-  if (defined($self->{Db}))
-  {
-    $self->{Db}->disconnect();
-    $self->{Db} = undef;
-  }
+  # The connection will be automatically closed. In a child process this
+  # will automatically do the right thing thanks to AutoInactiveDestroy.
+  $self->{Db} = undef;
 }
 
 sub UseDBIBackEnd($$$$$$)
@@ -610,9 +605,13 @@ sub UseDBIBackEnd($$$$$$)
 
   # The implementation assumes AutoCommit is on.
   $DbArgs->{AutoCommit} = 1;
+  # Make sure AutoInactiveDestroy is set so the database connection is not
+  # broken if we fork() (including if it's behind our back).
+  $DbArgs->{AutoInactiveDestroy} = 1;
 
   my $BackEnd = $class->new();
   $BackEnd->{ConnectArgs} = [$DbSource, $DbUser, $DbPassword, $DbArgs];
   AddDBBackEnd($DbSelector, $BackEnd);
 }
+
 1;
diff --git a/testbot/lib/WineTestBot/VMs.pm b/testbot/lib/WineTestBot/VMs.pm
index bb9a832..d893e49 100644
--- a/testbot/lib/WineTestBot/VMs.pm
+++ b/testbot/lib/WineTestBot/VMs.pm
@@ -344,9 +344,6 @@ sub Run($$$$$)
   # So set ChildPid in the parent and synchronize with the child so it only
   # starts once this is done.
 
-  # Make sure the child process will use its own database connections
-  CloseAllDBBackEnds();
-
   use Fcntl;
   my ($fd_read, $fd_write);
   pipe($fd_read, $fd_write); # For synchronization
@@ -378,6 +375,9 @@ sub Run($$$$$)
     return undef;
   }
 
+  # Close the database connections
+  CloseAllDBBackEnds();
+
   # Wait for the parent to set $VM->ChildPid
   close($fd_write);
   sysread($fd_read, $fd_write, 1);
@@ -389,6 +389,7 @@ sub Run($$$$$)
   require WineTestBot::VMs;
   my $VM = WineTestBot::VMs::CreateVMs()->GetItem($self->GetKey());
   exit 1 if (($VM->ChildPid || 0) != $$);
+  # Close the database connection we just opened
   $self->GetBackEnd()->Close();
 
   require WineTestBot::Log;