From 437c8744074a14e5d8401e2a561dc4693ba6620d Mon Sep 17 00:00:00 2001 From: Brian Moon Date: Wed, 22 Feb 2023 16:13:17 -0600 Subject: [PATCH 1/2] 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 --- Net/Gearman/Client.php | 66 ++++++++++++++++++++++++++++------ Net/Gearman/Worker.php | 10 +++++- composer.json | 2 +- phpunit.xml.dist | 35 +++++++++--------- phpunit.xml.functional-dist | 39 ++++++++++---------- tests/Net/Gearman/TaskTest.php | 10 ++---- tests/README.md | 3 +- 7 files changed, 107 insertions(+), 58 deletions(-) diff --git a/Net/Gearman/Client.php b/Net/Gearman/Client.php index a21a3ec4..3288325e 100644 --- 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 index 799fc56f..6a06786d 100644 --- a/Net/Gearman/Worker.php +++ b/Net/Gearman/Worker.php @@ -48,7 +48,7 @@ * exit; * } * - * ?> + * * * * @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 index ba37ccbd..ee6066ea 100644 --- 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 index 275d8d62..a0a68262 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,19 +1,18 @@ - - - - ./tests - - - - - functional - - - - - ./Net - - - \ No newline at end of file + + + + ./Net + + + + + ./tests + + + + + functional + + + diff --git a/phpunit.xml.functional-dist b/phpunit.xml.functional-dist index 503f00a9..be69e729 100644 --- a/phpunit.xml.functional-dist +++ b/phpunit.xml.functional-dist @@ -1,22 +1,21 @@ - - - - - - - ./tests - - - - - functional - - - - - ./Net - - + + + + ./Net + + + + + + + + ./tests + + + + + functional + + diff --git a/tests/Net/Gearman/TaskTest.php b/tests/Net/Gearman/TaskTest.php index 58f07585..b447a5f2 100644 --- 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 index 2f2a8b37..c29362b8 100644 --- 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 From 38fec5d426ccd2141a607caf4e11f22729eeb625 Mon Sep 17 00:00:00 2001 From: Brian Moon Date: Fri, 24 Feb 2023 11:21:12 -0600 Subject: [PATCH 2/2] Use a better method for checking if a server is connected --- Net/Gearman/Worker.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Net/Gearman/Worker.php b/Net/Gearman/Worker.php index 6a06786d..a4c9f964 100644 --- a/Net/Gearman/Worker.php +++ b/Net/Gearman/Worker.php @@ -275,6 +275,7 @@ private function connect($server) { if (isset($this->retryConn[$server])) { unset($this->retryConn[$server]); + $this->status("Removing server from the retry list.", $server); } $this->status("Connected to $server", $server); @@ -869,7 +870,7 @@ protected function status($message, $server = null) if (!empty($server)) { $failed_conns = isset($this->failedConn[$server]) ? $this->failedConn[$server] : 0; - $connected = isset($this->retryConn[$server]); + $connected = isset($this->conn[$server]) && $this->conn[$server]->isConnected(); } else { $failed_conns = null; $connected = null;