Master Server|Fixed: Open write lock on server database

It was possible for a MasterServer to get itself into an invalid state which left an open write-lock on the server database. MasterServer has been refactored to always close any open file/locks automatically when no more references to the instance handle exist. Also log any error that occurs during either a server database or XML feed update to the main error log.
skyjake · May 13, 2012 · f855bdc · f855bdc
1 parent cc44c0c
commit f855bdc
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 44 deletions.
diff --git a/web/classes/masterserver.class.php b/web/classes/masterserver.class.php
@@ -33,6 +33,10 @@ function get_ident($info)
 {
     if(!is_array($info))
         throw new Exception('Invalid info argument, array expected.');
+    if(!isset($info['at']))
+        throw new Exception('Invalid info, parameter \'at\' not specified.');
+    if(!isset($info['port']))
+        throw new Exception('Invalid info, parameter \'port\' not specified.');
 
     return $info['at'] . ":" . $info['port'];
 }
@@ -52,24 +56,29 @@ class MasterServer
     public $servers;
     public $lastUpdate;
 
-    private $writable;
+    private $isWritable;
     private $file;
 
     public function __construct($writable=false)
     {
-        global $HTTP_SERVER_VARS;
-        $this->writable = $writable;
-        $this->file = fopen_recursive(self::DATA_FILE, $writable? 'r+' : 'r');
+        $this->isWritable = $writable;
+        $this->file = fopen_recursive(self::DATA_FILE, $this->isWritable? 'r+' : 'r');
         if(!$this->file) die();
         $this->lastUpdate = @filemtime(self::DATA_FILE);
         $this->dbLock();
         $this->load();
-        if(!$writable) $this->dbUnlock();
+        if(!$this->isWritable) $this->dbUnlock();
+    }
+
+    public function __destruct()
+    {
+        // If writable and the data file is open; save it and/or close.
+        $this->close();
     }
 
     private function dbLock()
     {
-        flock($this->file, $this->writable? 2 : 1);
+        flock($this->file, $this->isWritable? 2 : 1);
     }
 
     private function dbUnlock()
@@ -121,18 +130,30 @@ private function save()
 
     function insert($info)
     {
-        $this->servers[get_ident($info)] = $info;
+        try
+        {
+            $this->servers[get_ident($info)] = $info;
+        }
+        catch(Exception $e)
+        {
+            $errorMsg = 'Unhandled exception "'. $e->getMessage() .'" in '. __CLASS__ .'::'. __METHOD__ .'().';
+            trigger_error($errorMsg);
+        }
     }
 
     function close()
     {
-        if($this->writable)
+        if(!$this->file) return;
+
+        if($this->isWritable)
         {
             $this->save();
             $this->dbUnlock();
-            $this->writable = false;
+            $this->isWritable = false;
         }
+
         fclose($this->file);
+        $this->file = 0;
     }
 
     /**
@@ -255,8 +276,6 @@ private function updateXmlLog($includeDTD=true)
         flock($logFile, 3);
         fclose($logFile);
 
-        $this->close();
-
         return TRUE;
     }
 
@@ -275,10 +294,10 @@ public function xmlLogFile()
         }
         catch(Exception $e)
         {
-            // @todo log the error.
+            $errorMsg = 'Unhandled exception "'. $e->getMessage() .'" in '. __CLASS__ .'::'. __METHOD__ .'().';
+            trigger_error($errorMsg);
             return false;
         }
-
         return self::XML_LOG_FILE;
     }
 }
diff --git a/web/master.php b/web/master.php
@@ -26,27 +26,13 @@
 
 require_once('classes/masterserver.class.php');
 
-// Global Master Server instance.
-$ms = NULL;
-
-function MasterServer_inst()
-{
-    global $ms;
-    if(is_null($ms))
-    {
-        $ms = new MasterServer(true);
-    }
-    return $ms;
-}
-
 function write_server($info)
 {
     // We will not write empty infos.
     if(count($info) <= 10) return;
 
-    $ms = MasterServer_inst();
+    $ms = new MasterServer(true/*writable*/);
     $ms->insert($info);
-    $ms->close();
 }
 
 function update_server($announcement, $addr)
@@ -87,8 +73,7 @@ function update_server($announcement, $addr)
 
 function answer_request()
 {
-    $ms = MasterServer_inst();
-
+    $ms = new MasterServer();
     while(list($ident, $info) = each($ms->servers))
     {
         while(list($label, $value) = each($info))
@@ -98,10 +83,27 @@ function answer_request()
         // An empty line ends the server.
         print "\n";
     }
-    $ms->close();
 }
 
-$query  = $HTTP_SERVER_VARS['QUERY_STRING'];
+function return_xmllog()
+{
+    $ms = new MasterServer();
+    // Update the log if necessary.
+    $logPath = $ms->xmlLogFile();
+
+    if($logPath !== false)
+    {
+        $result = file_get_contents_utf8($logPath, 'text/xml', 'utf-8');
+        if($result !== false)
+        {
+            // Return the log to the client.
+            header("Content-Type: text/xml; charset=utf-8");
+            echo mb_ereg_replace('http', 'https', $result);
+        }
+    }
+}
+
+$query = $HTTP_SERVER_VARS['QUERY_STRING'];
 
 // There are four operating modes:
 // 1. Server announcement processing.
@@ -111,27 +113,18 @@ function answer_request()
 
 if(isset($GLOBALS['HTTP_RAW_POST_DATA']) && !$query)
 {
+    $announcement = $GLOBALS['HTTP_RAW_POST_DATA'];
     $remote = $HTTP_SERVER_VARS['REMOTE_ADDR'];
 
-    update_server($GLOBALS['HTTP_RAW_POST_DATA'], $remote);
+    update_server($announcement, $remote);
 }
 else if($query == 'list')
 {
     answer_request();
 }
 else if($query == 'xml')
 {
-    $ms = MasterServer_inst();
-
-    $logPath = $ms->xmlLogFile();
-    if($logPath === false) exit;
-
-    $result = file_get_contents_utf8($logPath, 'text/xml', 'utf-8');
-    if($result === false) exit; // Most peculiar...
-
-    // Return the log to the client.
-    header("Content-Type: text/xml; charset=utf-8");
-    echo mb_ereg_replace('http', 'https', $result);
+    return_xmllog();
 }
 else
 {