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

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

Merged
merged 6 commits into from Aug 18, 2022
Merged

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

merged 6 commits into from Aug 18, 2022

Conversation

daannet
Copy link
Contributor

@daannet daannet commented Aug 17, 2022

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
Copy link
Contributor

DougSisk commented Aug 17, 2022

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.

@daannet
Copy link
Contributor Author

daannet commented Aug 17, 2022

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
Member

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

@DougSisk
Copy link
Contributor

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
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
Copy link
Contributor Author

daannet commented Aug 18, 2022

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
Copy link
Contributor

DougSisk commented Aug 18, 2022

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.

None yet

3 participants