Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 2 deletions src/Illuminate/Broadcasting/UniqueBroadcastEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ class UniqueBroadcastEvent extends BroadcastEvent implements ShouldBeUnique
*/
public function __construct($event)
{
$this->uniqueId = get_class($event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this need to be removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, yes. Removing it avoids duplicating the event class name in the lock key.

BroadcastEvent::displayName() already returns get_class($this->event):

/**
* Get the display name for the queued job.
*
* @return string
*/
public function displayName()
{
return get_class($this->event);
}

So with the updated UniqueLock::getKey() now using displayName() for the job identifier, keeping that line would result in:

laravel_unique_job:App\Events\MyEvent:App\Events\MyEvent123

Instead of:

laravel_unique_job:App\Events\MyEvent:123

BroadcastManagerTest verifies this expected lock key format in these 3 tests:

public function testUniqueEventsCanBeBroadcast()
{
Bus::fake();
Queue::fake();
Broadcast::queue(new TestEventUnique);
Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);
$lockKey = 'laravel_unique_job:'.TestEventUnique::class.':';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}
public function testUniqueEventsCanBeBroadcastWithUniqueIdFromProperty()
{
Bus::fake();
Queue::fake();
Broadcast::queue(new TestEventUniqueWithIdProperty);
Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);
$lockKey = 'laravel_unique_job:'.TestEventUniqueWithIdProperty::class.':unique-id-property';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}
public function testUniqueEventsCanBeBroadcastWithUniqueIdFromMethod()
{
Bus::fake();
Queue::fake();
Broadcast::queue(new TestEventUniqueWithIdMethod);
Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);
$lockKey = 'laravel_unique_job:'.TestEventUniqueWithIdMethod::class.':unique-id-method';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}

This relates to my earlier comment (#57499 (comment)); if multiple jobs share the same displayName(), this PR could introduce lock collisions. I can update it to concatenate both class name and displayName() as proposed there if you prefer that approach.


if (method_exists($event, 'uniqueId')) {
$this->uniqueId .= $event->uniqueId();
} elseif (property_exists($event, 'uniqueId')) {
Expand Down
6 changes: 5 additions & 1 deletion src/Illuminate/Bus/UniqueLock.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ public static function getKey($job)
? $job->uniqueId()
: ($job->uniqueId ?? '');

return 'laravel_unique_job:'.get_class($job).':'.$uniqueId;
$jobName = method_exists($job, 'displayName')
? $job->displayName()
: get_class($job);

return 'laravel_unique_job:'.$jobName.':'.$uniqueId;
}
}
6 changes: 5 additions & 1 deletion src/Illuminate/Queue/Middleware/ThrottlesExceptions.php
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,11 @@ protected function getKey($job)
return $this->prefix.$job->job->uuid();
}

return $this->prefix.hash('xxh128', get_class($job));
$jobName = method_exists($job, 'displayName')
? $job->displayName()
: get_class($job);

return $this->prefix.hash('xxh128', $jobName);
}

/**
Expand Down
12 changes: 9 additions & 3 deletions src/Illuminate/Queue/Middleware/WithoutOverlapping.php
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,14 @@ public function shared()
*/
public function getLockKey($job)
{
return $this->shareKey
? $this->prefix.$this->key
: $this->prefix.get_class($job).':'.$this->key;
if ($this->shareKey) {
return $this->prefix.$this->key;
}

$jobName = method_exists($job, 'displayName')
? $job->displayName()
: get_class($job);

return $this->prefix.$jobName.':'.$this->key;
}
}
40 changes: 39 additions & 1 deletion tests/Integration/Broadcasting/BroadcastManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,35 @@ public function testUniqueEventsCanBeBroadcast()
Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);

$lockKey = 'laravel_unique_job:'.UniqueBroadcastEvent::class.':'.TestEventUnique::class;
$lockKey = 'laravel_unique_job:'.TestEventUnique::class.':';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}

public function testUniqueEventsCanBeBroadcastWithUniqueIdFromProperty()
{
Bus::fake();
Queue::fake();

Broadcast::queue(new TestEventUniqueWithIdProperty);

Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);

$lockKey = 'laravel_unique_job:'.TestEventUniqueWithIdProperty::class.':unique-id-property';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}

public function testUniqueEventsCanBeBroadcastWithUniqueIdFromMethod()
{
Bus::fake();
Queue::fake();

Broadcast::queue(new TestEventUniqueWithIdMethod);

Bus::assertNotDispatched(UniqueBroadcastEvent::class);
Queue::assertPushed(UniqueBroadcastEvent::class);

$lockKey = 'laravel_unique_job:'.TestEventUniqueWithIdMethod::class.':unique-id-method';
$this->assertFalse($this->app->get(Cache::class)->lock($lockKey, 10)->get());
}

Expand Down Expand Up @@ -178,6 +206,16 @@ public function broadcastOn()
}
}

class TestEventUniqueWithIdProperty extends TestEventUnique
{
public string $uniqueId = 'unique-id-property';
}

class TestEventUniqueWithIdMethod extends TestEventUnique
{
public string $uniqueId = 'unique-id-method';
}

class TestEventRescue implements ShouldBroadcast, ShouldRescue
{
/**
Expand Down
80 changes: 80 additions & 0 deletions tests/Integration/Queue/ThrottlesExceptionsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Exception;
use Illuminate\Bus\Dispatcher;
use Illuminate\Bus\Queueable;
use Illuminate\Cache\RateLimiter;
use Illuminate\Contracts\Debug\ExceptionHandler;
use Illuminate\Contracts\Queue\Job;
use Illuminate\Queue\CallQueuedHandler;
Expand Down Expand Up @@ -345,6 +346,85 @@ public function release()
$middleware->report(fn () => false);
$middleware->handle($job, $next);
}

public function testUsesJobClassNameForCacheKey()
{
$rateLimiter = $this->mock(RateLimiter::class);

$job = new class
{
public $released = false;

public function release()
{
$this->released = true;

return $this;
}
};

$expectedKey = 'laravel_throttles_exceptions:'.hash('xxh128', get_class($job));

$rateLimiter->shouldReceive('tooManyAttempts')
->once()
->with($expectedKey, 10)
->andReturn(false);

$rateLimiter->shouldReceive('hit')
->once()
->with($expectedKey, 600);

$next = function ($job) {
throw new RuntimeException('Whoops!');
};

$middleware = new ThrottlesExceptions();
$middleware->handle($job, $next);

$this->assertTrue($job->released);
}

public function testUsesDisplayNameForCacheKeyWhenAvailable()
{
$rateLimiter = $this->mock(RateLimiter::class);

$job = new class
{
public $released = false;

public function release()
{
$this->released = true;

return $this;
}

public function displayName(): string
{
return 'App\\Actions\\ThrottlesExceptionsTestAction';
}
};

$expectedKey = 'laravel_throttles_exceptions:'.hash('xxh128', 'App\\Actions\\ThrottlesExceptionsTestAction');

$rateLimiter->shouldReceive('tooManyAttempts')
->once()
->with($expectedKey, 10)
->andReturn(false);

$rateLimiter->shouldReceive('hit')
->once()
->with($expectedKey, 600);

$next = function ($job) {
throw new RuntimeException('Whoops!');
};

$middleware = new ThrottlesExceptions();
$middleware->handle($job, $next);

$this->assertTrue($job->released);
}
}

class CircuitBreakerTestJob
Expand Down
86 changes: 86 additions & 0 deletions tests/Integration/Queue/UniqueJobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Exception;
use Illuminate\Bus\Queueable;
use Illuminate\Bus\UniqueLock;
use Illuminate\Container\Container;
use Illuminate\Contracts\Cache\Repository as Cache;
use Illuminate\Contracts\Queue\ShouldBeUnique;
Expand Down Expand Up @@ -169,6 +170,62 @@ protected function getLockKey($job)
{
return 'laravel_unique_job:'.(is_string($job) ? $job : get_class($job)).':';
}

public function testLockUsesDisplayNameWhenAvailable()
{
Bus::fake();

$lockKey = 'laravel_unique_job:App\\Actions\\UniqueTestAction:';

dispatch(new UniqueTestJobWithDisplayName);
$this->runQueueWorkerCommand(['--once' => true]);
Bus::assertDispatched(UniqueTestJobWithDisplayName::class);

$this->assertFalse(
$this->app->get(Cache::class)->lock($lockKey, 10)->get()
);

Bus::assertDispatchedTimes(UniqueTestJobWithDisplayName::class);
dispatch(new UniqueTestJobWithDisplayName);
$this->runQueueWorkerCommand(['--once' => true]);
Bus::assertDispatchedTimes(UniqueTestJobWithDisplayName::class);

$this->assertFalse(
$this->app->get(Cache::class)->lock($lockKey, 10)->get()
);
}

public function testUniqueLockCreatesKeyWithClassName()
{
$this->assertEquals(
'laravel_unique_job:'.UniqueTestJob::class.':',
UniqueLock::getKey(new UniqueTestJob)
);
}

public function testUniqueLockCreatesKeyWithIdAndClassName()
{
$this->assertEquals(
'laravel_unique_job:'.UniqueIdTestJob::class.':unique-id-1',
UniqueLock::getKey(new UniqueIdTestJob)
);
}

public function testUniqueLockCreatesKeyWithDisplayNameWhenAvailable()
{
$this->assertEquals(
'laravel_unique_job:App\\Actions\\UniqueTestAction:unique-id-2',
UniqueLock::getKey(new UniqueIdTestJobWithDisplayName)
);
}

public function testUniqueLockCreatesKeyWithIdAndDisplayNameWhenAvailable()
{
$this->assertEquals(
'laravel_unique_job:App\\Actions\\UniqueTestAction:unique-id-2',
UniqueLock::getKey(new UniqueIdTestJobWithDisplayName)
);
}
}

class UniqueTestJob implements ShouldQueue, ShouldBeUnique
Expand Down Expand Up @@ -239,3 +296,32 @@ public function uniqueVia(): Cache
return Container::getInstance()->make(Cache::class);
}
}

class UniqueIdTestJob extends UniqueTestJob
{
public function uniqueId(): string
{
return 'unique-id-1';
}
}

class UniqueTestJobWithDisplayName extends UniqueTestJob
{
public function displayName(): string
{
return 'App\\Actions\\UniqueTestAction';
}
}

class UniqueIdTestJobWithDisplayName extends UniqueTestJob
{
public function uniqueId(): string
{
return 'unique-id-2';
}

public function displayName(): string
{
return 'App\\Actions\\UniqueTestAction';
}
}
33 changes: 33 additions & 0 deletions tests/Integration/Queue/WithoutOverlappingJobsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,31 @@ public function testGetLock()
(new WithoutOverlapping('key'))->withPrefix('prefix:')->shared()->getLockKey($job)
);
}

public function testGetLockUsesDisplayName()
{
$job = new OverlappingTestJobWithDisplayName;

$this->assertSame(
'laravel-queue-overlap:App\\Actions\\WithoutOverlappingTestAction:key',
(new WithoutOverlapping('key'))->getLockKey($job)
);

$this->assertSame(
'laravel-queue-overlap:key',
(new WithoutOverlapping('key'))->shared()->getLockKey($job)
);

$this->assertSame(
'prefix:App\\Actions\\WithoutOverlappingTestAction:key',
(new WithoutOverlapping('key'))->withPrefix('prefix:')->getLockKey($job)
);

$this->assertSame(
'prefix:key',
(new WithoutOverlapping('key'))->withPrefix('prefix:')->shared()->getLockKey($job)
);
}
}

class OverlappingTestJob
Expand Down Expand Up @@ -221,3 +246,11 @@ public function middleware()
return [(new WithoutOverlapping)->shared()];
}
}

class OverlappingTestJobWithDisplayName extends OverlappingTestJob
{
public function displayName(): string
{
return 'App\\Actions\\WithoutOverlappingTestAction';
}
}