Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Fixes Batch Callbacks not triggering if job timeout while in transaction #48961

Merged
merged 33 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e8117e9
wip
crynobone Nov 8, 2023
ef4db02
wip
crynobone Nov 8, 2023
e591c19
wip
crynobone Nov 8, 2023
a9b412d
wip
crynobone Nov 8, 2023
9f5eee9
wip
crynobone Nov 9, 2023
73a5423
wip
crynobone Nov 9, 2023
c8fb690
wip
crynobone Nov 9, 2023
9206e7c
wip
crynobone Nov 9, 2023
b52fd9c
wip
crynobone Nov 9, 2023
e7d6ba9
Apply fixes from StyleCI
StyleCIBot Nov 9, 2023
bd0ec28
wip
crynobone Nov 9, 2023
f5e7948
Merge remote-tracking branch 'upstream/fixes-48392' into fixes-48392
crynobone Nov 9, 2023
0a6d4ce
wip
crynobone Nov 9, 2023
276b799
Apply fixes from StyleCI
StyleCIBot Nov 9, 2023
cd4343e
wip
crynobone Nov 10, 2023
7a6c41c
Merge remote-tracking branch 'upstream/fixes-48392' into fixes-48392
crynobone Nov 10, 2023
2dbc195
Apply fixes from StyleCI
StyleCIBot Nov 10, 2023
5a99716
wip
crynobone Nov 10, 2023
ffeeb80
Merge remote-tracking branch 'upstream/fixes-48392' into fixes-48392
crynobone Nov 10, 2023
b63a089
wip
crynobone Nov 10, 2023
3e9461d
wip
crynobone Nov 10, 2023
45e8683
Update BatchableTransactionTest.php
crynobone Nov 10, 2023
a283b83
Apply fixes from StyleCI
StyleCIBot Nov 10, 2023
545c3d2
wip
crynobone Nov 10, 2023
d41bf1e
wip
crynobone Nov 10, 2023
44b5a86
formatting
taylorotwell Nov 10, 2023
0b05a14
Apply fixes from StyleCI
StyleCIBot Nov 10, 2023
d6deac5
formatting
taylorotwell Nov 10, 2023
005ad11
formatting
taylorotwell Nov 10, 2023
781da67
formatting
taylorotwell Nov 10, 2023
c43721a
Apply fixes from StyleCI
StyleCIBot Nov 10, 2023
500ac8f
fix test
taylorotwell Nov 10, 2023
ef95559
Merge branch 'fixes-48392' of github.com:laravel/framework into fixes…
taylorotwell Nov 10, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
"league/flysystem-sftp-v3": "^3.0",
"mockery/mockery": "^1.5.1",
"nyholm/psr7": "^1.2",
"orchestra/testbench-core": "^8.12",
"orchestra/testbench-core": "^8.15.1",
"pda/pheanstalk": "^4.0",
"phpstan/phpstan": "^1.4.7",
"phpunit/phpunit": "^10.0.7",
Expand Down
3 changes: 3 additions & 0 deletions src/Illuminate/Bus/BatchRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

use Closure;

/**
* @method void rollBack()
*/
interface BatchRepository
{
/**
Expand Down
10 changes: 10 additions & 0 deletions src/Illuminate/Bus/DatabaseBatchRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,16 @@ public function transaction(Closure $callback)
return $this->connection->transaction(fn () => $callback());
}

/**
* Rollback the last database transaction for the connection.
*
* @return void
*/
public function rollBack()
{
$this->connection->rollBack();
}

/**
* Serialize the given value.
*
Expand Down
23 changes: 23 additions & 0 deletions src/Illuminate/Queue/Jobs/Job.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@

namespace Illuminate\Queue\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\BatchRepository;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Queue\Events\JobFailed;
use Illuminate\Queue\ManuallyFailedException;
use Illuminate\Queue\TimeoutExceededException;
use Illuminate\Support\InteractsWithTime;
use Throwable;

abstract class Job
{
Expand Down Expand Up @@ -183,6 +187,25 @@ public function fail($e = null)
return;
}

$commandName = $this->payload()['data']['commandName'] ?? false;

// If the exception is due to a job timing out, we need to rollback the current
// database transaction so that the failed job count can be incremented with
// the proper value. Otherwise, the current transaction will never commit.
if ($e instanceof TimeoutExceededException &&
$commandName &&
in_array(Batchable::class, class_uses_recursive($commandName))) {
$batchRepository = $this->resolve(BatchRepository::class);

if (method_exists($batchRepository, 'rollBack')) {
try {
$batchRepository->rollBack();
} catch (Throwable $e) {
// ...
}
}
}

try {
// If the job has failed, we will delete it, call the "failed" method and then call
// an event indicating the job has failed so it can be logged if needed. This is
Expand Down
65 changes: 65 additions & 0 deletions tests/Integration/Database/Queue/BatchableTransactionTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

namespace Illuminate\Tests\Integration\Database\Queue;

use Illuminate\Foundation\Testing\DatabaseMigrations;
use Illuminate\Support\Facades\Bus;
use Illuminate\Support\Facades\DB;
use Illuminate\Tests\Integration\Database\DatabaseTestCase;
use Orchestra\Testbench\Attributes\WithMigration;
use PHPUnit\Framework\Attributes\RequiresPhpExtension;
use Symfony\Component\Process\Exception\ProcessSignaledException;
use Throwable;

use function Orchestra\Testbench\remote;

#[RequiresPhpExtension('pcntl')]
#[WithMigration('laravel', 'queue')]
class BatchableTransactionTest extends DatabaseTestCase
{
use DatabaseMigrations;

protected function defineEnvironment($app)
{
parent::defineEnvironment($app);

$config = $app['config'];

if ($config->get('database.default') === 'testing') {
$this->markTestSkipped('Test does not support using :memory: database connection');
}

$config->set(['queue.default' => 'database']);
}

public function testItCanHandleTimeoutJob()
{
Bus::batch([new Fixtures\TimeOutJobWithTransaction()])
->allowFailures()
->dispatch();

$this->assertSame(1, DB::table('jobs')->count());
$this->assertSame(0, DB::table('failed_jobs')->count());
$this->assertSame(1, DB::table('job_batches')->count());

try {
remote('queue:work --stop-when-empty', [
'DB_CONNECTION' => config('database.default'),
'QUEUE_CONNECTION' => config('queue.default'),
])->run();
} catch (Throwable $e) {
$this->assertInstanceOf(ProcessSignaledException::class, $e);
$this->assertSame('The process has been signaled with signal "9".', $e->getMessage());
}

$this->assertSame(0, DB::table('jobs')->count());
$this->assertSame(1, DB::table('failed_jobs')->count());

$this->assertDatabaseHas('job_batches', [
'total_jobs' => 1,
'pending_jobs' => 1,
'failed_jobs' => 1,
'failed_job_ids' => json_encode(DB::table('failed_jobs')->pluck('uuid')->all()),
]);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

namespace Illuminate\Tests\Integration\Database\Queue\Fixtures;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Support\Facades\DB;

class TimeOutJobWithTransaction implements ShouldQueue
{
use InteractsWithQueue, Queueable, Batchable;

public int $tries = 1;
public int $timeout = 2;

public function handle(): void
{
DB::transaction(fn () => sleep(20));
}
}
2 changes: 1 addition & 1 deletion tests/Queue/QueueBeanstalkdJobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function testFireProperlyCallsTheJobHandler()
public function testFailProperlyCallsTheJobHandler()
{
$job = $this->getJob();
$job->getPheanstalkJob()->shouldReceive('getData')->once()->andReturn(json_encode(['job' => 'foo', 'uuid' => 'test-uuid', 'data' => ['data']]));
$job->getPheanstalkJob()->shouldReceive('getData')->andReturn(json_encode(['job' => 'foo', 'uuid' => 'test-uuid', 'data' => ['data']]));
$job->getContainer()->shouldReceive('make')->once()->with('foo')->andReturn($handler = m::mock(BeanstalkdJobTestFailedTest::class));
$job->getPheanstalk()->shouldReceive('delete')->once()->with($job->getPheanstalkJob())->andReturnSelf();
$handler->shouldReceive('failed')->once()->with(['data'], m::type(Exception::class), 'test-uuid');
Expand Down