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] Add unique locking to broadcast events #43416

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

DougSisk
Copy link
Contributor

Queueing broadcast events is incredibly important, especially at scale. Unfortunately at the moment there is no out-of-the-box method to prevent queues from being overloaded with repeated events. This can lead to redundant broadcast events that eat up data allowances.

I'm proposing a new Illuminate\Contracts\Broadcasting\ShouldBeUnique contract that tells the dispatcher to use a separate UniqueBroadcastEvent class. If no uniqueId method or property is defined, the event's class name is used as the identifier. You can also set uniqueFor and uniqueVia like other unique queue jobs via a property or method.

@taylorotwell taylorotwell merged commit d20d7ef into laravel:9.x Jul 26, 2022
@daannet
Copy link
Contributor

daannet commented Jul 27, 2022

In the test case the lock is successfully acquired after queueing the event, which shouldn't be possible if the lock was acquired. Looks like this isn't a complete solution yet.

@driesvints
Copy link
Member

@DougSisk

@taylorotwell
Copy link
Member

@DougSisk any thoughts here?

@DougSisk
Copy link
Contributor Author

DougSisk commented Aug 2, 2022

It seems like since the broadcast manager doesn't use a dispatcher it's pushing right onto the queue. I'm guessing the best option would be to acquire the lock within BroadcastManager\queue() and continue to not use a dispatcher?

Ken-vdE pushed a commit to Ken-vdE/framework that referenced this pull request Aug 9, 2022
* Add unique locking to broadcast events

* Don't require uniqueId, set default

* Locking irrelevant when broadcasting now

* formatting

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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.

4 participants