Improvements from DealNews upstream

* Add status callback to Client to passively know when connections fail * Add a sleepConnection call in worker when no job is returned from the job server * Initialize job params when empty to avoid notice * Update phpunit
brianlmoon · Feb 22, 2023 · 437c874 · 437c874
1 parent 0f61d29
commit 437c874
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 58 deletions.
diff --git a/Net/Gearman/Client.php b/Net/Gearman/Client.php
@@ -57,6 +57,13 @@ class Net_Gearman_Client
      */
     protected $timeout = 1000;
 
+    /**
+     * Callbacks array for receiving connection status
+     *
+     * @var array $callback
+     */
+    protected $callback = array();
+
     /**
      * Constructor
      *
@@ -71,7 +78,7 @@ class Net_Gearman_Client
      */
     public function __construct($servers, $timeout = 1000)
     {
-        if (!is_array($servers) && strlen($servers)) {
+        if (!is_array($servers) && strlen($servers) > 0) {
             $servers = array($servers);
         } elseif (is_array($servers) && !count($servers)) {
             throw new Net_Gearman_Exception('Invalid servers specified');
@@ -108,7 +115,7 @@ protected function getConnection($uniq=null, $servers=null)
          */
         $tried_servers = array();
 
-        while ($conn === null && count($servers)) {
+        while ($conn === null && count($servers) > 0) {
             if (count($servers) === 1) {
                 $key = key($servers);
             } elseif ($uniq === null) {
@@ -121,10 +128,12 @@ protected function getConnection($uniq=null, $servers=null)
 
             $tried_servers[] = $server;
 
-            if (empty($this->conn[$server])) {
-                $conn = null;
+            if (empty($this->conn[$server]) || !$this->conn[$server]->isConnected()) {
 
+                $conn  = null;
                 $start = microtime(true);
+                $e     = null;
+
                 try {
                     $conn = new Net_Gearman_Connection($server, $this->timeout);
                 } catch (Net_Gearman_Exception $e) {
@@ -141,6 +150,17 @@ protected function getConnection($uniq=null, $servers=null)
                     break;
                 }
 
+                foreach ($this->callback as $callback) {
+                    call_user_func(
+                        $callback,
+                        $server,
+                        $conn !== null,
+                        $this->timeout,
+                        microtime(true) - $start,
+                        $e
+                    );
+                }
+
             } else {
                 $conn = $this->conn[$server];
             }
@@ -160,6 +180,22 @@ protected function getConnection($uniq=null, $servers=null)
         return $conn;
     }
 
+    /**
+     * Attach a callback for connection status
+     *
+     * @param callback $callback A valid PHP callback
+     *
+     * @return void
+     * @throws Net_Gearman_Exception When an invalid callback is specified.
+     */
+    public function attachCallback($callback)
+    {
+        if (!is_callable($callback)) {
+            throw new Net_Gearman_Exception('Invalid callback specified');
+        }
+        $this->callback[] = $callback;
+    }
+
     /**
      * Fire off a background task with the given arguments
      *
@@ -297,22 +333,26 @@ public function runSet(Net_Gearman_Set $set, $timeout = null)
                 $t++;
             }
 
-            $write  = null;
-            $except = null;
+            $write     = null;
+            $except    = null;
             $read_cons = array();
+
             foreach ($this->conn as $conn) {
                 $read_conns[] = $conn->socket;
             }
+
             @socket_select($read_conns, $write, $except, $socket_timeout);
-            foreach ($this->conn as $conn) {
+
+            $error_messages = [];
+
+            foreach ($this->conn as $server => $conn) {
                 $err = socket_last_error($conn->socket);
                 // Error 11 is EAGAIN and is normal in non-blocking mode
                 // Error 35 happens on macOS often enough to be annoying
                 if ($err && $err != 11 && $err != 35) {
                     $msg = socket_strerror($err);
-                    socket_getpeername($conn->socket, $remote_address, $remote_port);
-                    socket_getsockname($conn->socket, $local_address, $local_port);
-                    trigger_error("socket_select failed: ($err) $msg; server: $remote_address:$remote_port", E_USER_WARNING);
+                    list($remote_address, $remote_port) = explode(":", $server);
+                    $error_messages[] = "socket_select failed: ($err) $msg; server: $remote_address:$remote_port";
                 }
                 socket_clear_error($conn->socket);
                 $resp = $conn->read();
@@ -321,6 +361,10 @@ public function runSet(Net_Gearman_Set $set, $timeout = null)
                 }
             }
 
+            // if all connections threw errors, throw an exception
+            if (count($error_messages) == count($this->conn)) {
+                throw new Net_Gearman_Exception(implode("; ", $error_messages));
+            }
         }
     }
 
@@ -388,6 +432,8 @@ public function disconnect()
                 $conn->close();
             }
         }
+
+        $this->conn = [];
     }
 
     /**

diff --git a/Net/Gearman/Worker.php b/Net/Gearman/Worker.php
@@ -48,7 +48,7 @@
  *     exit;
  * }
  *
- * ?>
+ *
  * </code>
  *
  * @category  Net
@@ -707,6 +707,9 @@ protected function doWork($conn)
                     break;
                 }
             }
+
+            $this->sleepConnection($server);
+
             $this->status(
                 "No job was returned from the server",
                 $server
@@ -736,6 +739,11 @@ protected function doWork($conn)
         }
 
         try {
+
+            if (empty($this->initParams[$name])) {
+                $this->initParams[$name] = [];
+            }
+
             $job = Net_Gearman_Job::factory(
                 $name, $conn, $handle, $this->initParams[$name]
             );

diff --git a/composer.json b/composer.json
@@ -29,6 +29,6 @@
         "."
     ],
     "require-dev": {
-        "phpunit/phpunit": "^7.5|^8.0"
+        "phpunit/phpunit": "^9.6"
     }
 }
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -1,19 +1,18 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<phpunit bootstrap="./tests/bootstrap.php"
-         colors="true">
-    <testsuites>
-        <testsuite name="Main">
-            <directory suffix="Test.php">./tests</directory>
-        </testsuite>
-    </testsuites>
-    <groups>
-        <exclude>
-            <group>functional</group>
-        </exclude>
-    </groups>
-    <filter>
-        <whitelist>
-            <directory>./Net</directory>
-        </whitelist>
-    </filter>
-</phpunit>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bootstrap="./tests/bootstrap.php" colors="true" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
+  <coverage>
+    <include>
+      <directory>./Net</directory>
+    </include>
+  </coverage>
+  <testsuites>
+    <testsuite name="Main">
+      <directory suffix="Test.php">./tests</directory>
+    </testsuite>
+  </testsuites>
+  <groups>
+    <exclude>
+      <group>functional</group>
+    </exclude>
+  </groups>
+</phpunit>
diff --git a/phpunit.xml.functional-dist b/phpunit.xml.functional-dist
@@ -1,22 +1,21 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<phpunit bootstrap="./tests/bootstrap.php"
-         colors="true">
-    <php>
-        <const name="NET_GEARMAN_TEST_SERVER" value="localhost:4730"/>
-    </php>
-    <testsuites>
-        <testsuite name="Main">
-            <directory suffix="Test.php">./tests</directory>
-        </testsuite>
-    </testsuites>
-    <groups>
-        <include>
-            <group>functional</group>
-        </include>
-    </groups>
-    <filter>
-        <whitelist>
-            <directory>./Net</directory>
-        </whitelist>
-    </filter>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bootstrap="./tests/bootstrap.php" colors="true" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
+  <coverage>
+    <include>
+      <directory>./Net</directory>
+    </include>
+  </coverage>
+  <php>
+    <const name="NET_GEARMAN_TEST_SERVER" value="localhost:4730"/>
+  </php>
+  <testsuites>
+    <testsuite name="Main">
+      <directory suffix="Test.php">./tests</directory>
+    </testsuite>
+  </testsuites>
+  <groups>
+    <include>
+      <group>functional</group>
+    </include>
+  </groups>
 </phpunit>
diff --git a/tests/Net/Gearman/TaskTest.php b/tests/Net/Gearman/TaskTest.php
@@ -8,10 +8,10 @@ class Net_Gearman_TaskTest extends \PHPUnit\Framework\TestCase
      * Unknown job type.
      *
      * @return void
-     * @expectedException \Net_Gearman_Exception
      */
     public function testExceptionFromConstruct()
     {
+        $this->expectException(\Net_Gearman_Exception::class);
         new Net_Gearman_Task('foo', [], null, 8);
     }
 
@@ -30,20 +30,16 @@ public function testParameters()
         $this->assertEquals($uniq, $task->uniq);
     }
 
-    /**
-     * @expectedException \Net_Gearman_Exception
-     */
     public function testAttachInvalidCallback()
     {
+        $this->expectException(\Net_Gearman_Exception::class);
         $task = new Net_Gearman_Task('foo', []);
         $task->attachCallback('func_bar');
     }
 
-    /**
-     * @expectedException \Net_Gearman_Exception
-     */
     public function testAttachInvalidCallbackType()
     {
+        $this->expectException(\Net_Gearman_Exception::class);
         $task = new Net_Gearman_Task('foo', []);
         $this->assertInstanceOf('Net_Gearman_Task', $task->attachCallback('strlen', 666));
     }

diff --git a/tests/README.md b/tests/README.md
@@ -9,5 +9,6 @@ From the project root:
 
 ## Run the functional tests
 1. Start up your gearman job server
-1. Update the `NET_GEARMAN_TEST_SERVER` constant in `phpunit.xml.functional-dist` (if necessary)
+ 1. For local testing, this docker command can be used: ` docker run --name gearmand --rm -d -p 4730:4730 artefactual/gearmand:latest`
+1. Update the `NET_GEARMAN_TEST_SERVER` constant in `phpunit.xml.functional-dist` (if not on localhost and/or port 4730)
 1. vendor/bin/phpunit -c phpunit.xml.functional-dist