From 29cd433f1a9012e7df1dc2e97e03ef8551f2baf3 Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Thu, 18 Oct 2018 16:38:08 +0100 Subject: [PATCH 1/6] Add new exception and move existing into exceptions folder. --- .../EntityManagerClosedException.php | 2 +- src/Exceptions/QueueFailureException.php | 14 ++++++++++++++ src/QueueFailure.php | 10 ++++++++++ src/Worker.php | 1 + tests/WorkerTest.php | 2 +- 5 files changed, 27 insertions(+), 2 deletions(-) rename src/{ => Exceptions}/EntityManagerClosedException.php (65%) create mode 100644 src/Exceptions/QueueFailureException.php create mode 100644 src/QueueFailure.php diff --git a/src/EntityManagerClosedException.php b/src/Exceptions/EntityManagerClosedException.php similarity index 65% rename from src/EntityManagerClosedException.php rename to src/Exceptions/EntityManagerClosedException.php index a7fabc2..135fa12 100644 --- a/src/EntityManagerClosedException.php +++ b/src/Exceptions/EntityManagerClosedException.php @@ -1,7 +1,7 @@ Date: Thu, 18 Oct 2018 16:39:37 +0100 Subject: [PATCH 2/6] Move to PHP7.1 minimum version and implement changes to worker. --- composer.json | 2 +- src/Worker.php | 49 +++++++++++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/composer.json b/composer.json index 9104a49..ddd7195 100644 --- a/composer.json +++ b/composer.json @@ -9,7 +9,7 @@ } ], "require": { - "php": ">=5.6", + "php": ">=7.1", "illuminate/queue": "^5.3.24", "laravel-doctrine/orm": "^1.0" }, diff --git a/src/Worker.php b/src/Worker.php index 27f98a3..d1dbcff 100644 --- a/src/Worker.php +++ b/src/Worker.php @@ -1,5 +1,4 @@ -assertEntityManagerOpen(); } catch (EntityManagerClosedException $e) { if ($this->exceptions) { - $this->exceptions->report(new EntityManagerClosedException); + $this->exceptions->report($e); } return false; @@ -123,9 +125,7 @@ public function runNextJob($connectionName, $queue, WorkerOptions $options) $this->assertGoodDatabaseConnection(); try { - $job = $this->getNextJob( - $this->manager->connection($connectionName), $queue - ); + $job = $this->fetchNewJob($connectionName, $queue); // If we're able to pull a job off of the stack, we will process it and then return // from this method. If there is no job on the queue, we will "sleep" the worker @@ -138,20 +138,12 @@ public function runNextJob($connectionName, $queue, WorkerOptions $options) return true; } - } catch (Exception $e) { - if ($this->exceptions) { - $this->exceptions->report($e); - } - - if ($e instanceof QueueMustStop) { - return false; - } - } catch (Throwable $e) { + } catch (Exception | Throwable $e) { if ($this->exceptions) { $this->exceptions->report(new FatalThrowableError($e)); } - if ($e instanceof QueueMustStop) { + if ($e instanceof QueueMustStop || $e instanceof QueueFailure) { return false; } } @@ -187,4 +179,21 @@ private function assertGoodDatabaseConnection() $connection->connect(); } } + + /** + * @param string $connectionName + * @param null $queue + * @return \Illuminate\Contracts\Queue\Job|null + * @throws QueueFailureException + */ + private function fetchNewJob(string $connectionName, $queue): ?\Illuminate\Contracts\Queue\Job + { + try { + return $this->getNextJob( + $this->manager->connection($connectionName), $queue + ); + } catch (Exception | Throwable $e) { + throw new QueueFailureException($e); + } + } } From 86ec25839f1f7af02c58856c96d093e5be4ffca8 Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Fri, 19 Oct 2018 10:14:25 +0100 Subject: [PATCH 3/6] Move to 7.1 for the unit tests since this uses 7.1+ syntax --- .travis.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index bc37b4a..331554b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,13 +1,10 @@ language: php php: - - 5.6 - - 7.0 + - 7.1 matrix: fast_finish: true - allow_failures: - - php: 7.0 before_script: - composer install --no-interaction --dev --prefer-dist From 3961dca84259f277312a41da9590ae6eddac7d75 Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Fri, 19 Oct 2018 15:05:14 +0100 Subject: [PATCH 4/6] Fix unit tests to work with new exception throwing. --- tests/WorkerTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/WorkerTest.php b/tests/WorkerTest.php index b3e06cd..aab2200 100644 --- a/tests/WorkerTest.php +++ b/tests/WorkerTest.php @@ -19,6 +19,7 @@ use MaxBrokman\SafeQueue\Stopper; use MaxBrokman\SafeQueue\Worker; use Mockery as m; +use Symfony\Component\Debug\Exception\FatalThrowableError; class WorkerTest extends \PHPUnit_Framework_TestCase { @@ -160,7 +161,7 @@ public function testReconnected() // We must stop $this->stopper->shouldReceive('stop')->once(); // We must log this fact - $this->exceptions->shouldReceive('report')->with(m::type(BadThingHappened::class))->once(); + $this->exceptions->shouldReceive('report')->with(m::type(FatalThrowableError::class))->once(); // Make a job $job = m::mock(Job::class); @@ -192,7 +193,7 @@ public function testLoops() $jobTwo->shouldReceive('fire')->once()->andThrow(new BadThingHappened()); $jobTwo->shouldIgnoreMissing(); - $this->exceptions->shouldReceive('report')->with(m::type(BadThingHappened::class))->once(); + $this->exceptions->shouldReceive('report')->with(m::type(FatalThrowableError::class))->once(); $this->prepareToRunJob([$jobOne, $jobTwo]); From 60b768664561955be876f6c0e3b500a59e203356 Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Fri, 19 Oct 2018 15:17:43 +0100 Subject: [PATCH 5/6] Add new test to check the new functionality around queue read failures. --- tests/WorkerTest.php | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/WorkerTest.php b/tests/WorkerTest.php index aab2200..ce57af7 100644 --- a/tests/WorkerTest.php +++ b/tests/WorkerTest.php @@ -15,6 +15,7 @@ use Illuminate\Queue\Worker as IlluminateWorker; use Illuminate\Queue\WorkerOptions; use MaxBrokman\SafeQueue\Exceptions\EntityManagerClosedException; +use MaxBrokman\SafeQueue\Exceptions\QueueFailureException; use MaxBrokman\SafeQueue\QueueMustStop; use MaxBrokman\SafeQueue\Stopper; use MaxBrokman\SafeQueue\Worker; @@ -124,6 +125,21 @@ protected function prepareToRunJob($job) call_user_func_array([$popExpectation, 'andReturn'], $jobs); } + protected function prepareToRunJobFails($job) + { + if ($job instanceof Job) { + $jobs = [$job]; + } else { + $jobs = $job; + } + + $this->queueManager->shouldReceive('isDownForMaintenance')->andReturn(false); + $this->queueManager->shouldReceive('connection')->andReturn($this->queue); + $this->queueManager->shouldReceive('getName')->andReturn('test'); + + $this->queue->shouldReceive('pop')->andThrow(new QueueFailureException(new BadThingHappened())); + } + public function testExtendsLaravelWorker() { $this->assertInstanceOf(IlluminateWorker::class, $this->worker); @@ -215,6 +231,25 @@ public function testRestartsNicely() $this->worker->daemon('test', null, $this->options); } + + public function testQueueFailure() + { + // Entity manager will report open and good connection + $this->entityManager->shouldReceive('isOpen')->andReturn(true)->times(1); + $this->dbConnection->shouldReceive('ping')->andReturn(true)->times(1); + + // We must stop + $this->stopper->shouldReceive('stop')->once(); + + // Make a job + $job = m::mock(Job::class); + + $this->exceptions->shouldReceive('report')->with(m::type(FatalThrowableError::class))->once(); + + $this->prepareToRunJobFails([$job]); + + $this->worker->daemon('test', null, $this->options); + } } class BadThingHappened extends \Exception implements QueueMustStop From cc6db26f8d0211e3a5ec6e2ec147525e074ec81c Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Fri, 19 Oct 2018 15:52:10 +0100 Subject: [PATCH 6/6] Remove redundant code and add in a null job to increase coverage of the loop test. --- tests/WorkerTest.php | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/WorkerTest.php b/tests/WorkerTest.php index ce57af7..9e8e3ef 100644 --- a/tests/WorkerTest.php +++ b/tests/WorkerTest.php @@ -125,14 +125,8 @@ protected function prepareToRunJob($job) call_user_func_array([$popExpectation, 'andReturn'], $jobs); } - protected function prepareToRunJobFails($job) + protected function prepareToRunJobFails() { - if ($job instanceof Job) { - $jobs = [$job]; - } else { - $jobs = $job; - } - $this->queueManager->shouldReceive('isDownForMaintenance')->andReturn(false); $this->queueManager->shouldReceive('connection')->andReturn($this->queue); $this->queueManager->shouldReceive('getName')->andReturn('test'); @@ -193,8 +187,8 @@ public function testReconnected() public function testLoops() { // Entity manager will report open and good connection - $this->entityManager->shouldReceive('isOpen')->andReturn(true)->times(2); - $this->dbConnection->shouldReceive('ping')->andReturn(true)->times(2); + $this->entityManager->shouldReceive('isOpen')->andReturn(true)->times(3); + $this->dbConnection->shouldReceive('ping')->andReturn(true)->times(3); // We must stop $this->stopper->shouldReceive('stop')->once(); @@ -211,7 +205,7 @@ public function testLoops() $this->exceptions->shouldReceive('report')->with(m::type(FatalThrowableError::class))->once(); - $this->prepareToRunJob([$jobOne, $jobTwo]); + $this->prepareToRunJob([null, $jobOne, $jobTwo]); $this->worker->daemon('test', null, $this->options); } @@ -246,7 +240,7 @@ public function testQueueFailure() $this->exceptions->shouldReceive('report')->with(m::type(FatalThrowableError::class))->once(); - $this->prepareToRunJobFails([$job]); + $this->prepareToRunJobFails(); $this->worker->daemon('test', null, $this->options); }