Skip to content

[9.x] Fix unique lock release for broadcast events#43738

Merged
taylorotwell merged 6 commits into
laravel:9.xfrom
daannet:9.x
Aug 18, 2022
Merged

[9.x] Fix unique lock release for broadcast events#43738
taylorotwell merged 6 commits into
laravel:9.xfrom
daannet:9.x

Conversation

@daannet

@daannet daannet commented Aug 17, 2022

Copy link
Copy Markdown
Contributor

This fix fixes not triggered lock release of #43416, #43516. It added back the UniqueBroadcastEvent class created by @DougSisk but now the uniqueId will always include the original event class.

The regular BroadcastEvent does not implement the 'Queue' ShouldBeUnique and wasn't removing the lock. Now it is possible to dispatch the same event and id after the previous has been processed.

@DougSisk

DougSisk commented Aug 17, 2022

Copy link
Copy Markdown

This actually brings up another issue:

protected function ensureUniqueJobLockIsReleased($command)
{
if (! $command instanceof ShouldBeUnique) {
return;
}
$uniqueId = method_exists($command, 'uniqueId')
? $command->uniqueId()
: ($command->uniqueId ?? '');
$cache = method_exists($command, 'uniqueVia')
? $command->uniqueVia()
: $this->container->make(Cache::class);
$cache->lock(
'laravel_unique_job:'.get_class($command).$uniqueId
)->forceRelease();
}

The lock release for unique queue jobs should be moved to within the Illuminate\Bus\UniqueLock instead.

Comment thread src/Illuminate/Broadcasting/UniqueBroadcastEvent.php Outdated
@daannet

daannet commented Aug 17, 2022

Copy link
Copy Markdown
Contributor Author

This actually brings up another issue:

The UniqueBroadcastEvent has the data to release the lock after processing. Now uniqueVia is taken from the inner event or retrieved from container and working :)

The lock release for unique queue jobs should be moved to within the Illuminate\Bus\UniqueLock instead.

I don't know how desirable it is to change the queue/jobs logic for broadcast events. Where should we draw the line?

@taylorotwell

Copy link
Copy Markdown
Member

IMO we should not be modifying queue / job logic to accommodate this feature. Risks breaking more things.

@DougSisk

Copy link
Copy Markdown

IMO we should not be modifying queue / job logic to accommodate this feature. Risks breaking more things.

I've opened #43740 which explains the issue. It does not pertain to making this feature work; just fixing decoupled logic.

@taylorotwell

Copy link
Copy Markdown
Member

Can someone explain the actual problem at its root and how this fixes it? Also, calling the foundation app helper from component level code is generally not allowed.

@daannet

daannet commented Aug 18, 2022

Copy link
Copy Markdown
Contributor Author

Can someone explain the actual problem at its root and how this fixes it? Also, calling the foundation app helper from component level code is generally not allowed.

The current BroadcastEvent (job) doesn't have the queue ShouldBeUnique implementation and the required uniqueVia etc data and the unique lock is not released after processing.

@DougSisk

DougSisk commented Aug 18, 2022

Copy link
Copy Markdown

I believe this is only an issue if you don't define a $uniqueFor, which is why I'm personally not seeing abnormal behavior using this in production.

@taylorotwell taylorotwell merged commit d02865f into laravel:9.x Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants