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

[6.x] Broadcaster - Improve ShouldBroadcastNow performance #30797

Merged
merged 1 commit into from
Dec 10, 2019
Merged

[6.x] Broadcaster - Improve ShouldBroadcastNow performance #30797

merged 1 commit into from
Dec 10, 2019

Conversation

josiasmontag
Copy link
Contributor

Problem

Currently, ShouldBroadcastNow sends the BroadcastEvent to the sync queue. This causes some performance issues:

  • It serializes and directly afterwards unserializes the BroadcastEvent
  • In combination with the SerializesModels Trait it creates completely unnecessary database queries

Solution

We could just directly dispatchNow() the BroadcastEvent. There should be no reason at all to send it to the the sync queue.

@taylorotwell taylorotwell merged commit c5c5b75 into laravel:6.x Dec 10, 2019
@taylorotwell
Copy link
Member

Broadcast manager wasn't being tested so I added some.

@josiasmontag josiasmontag deleted the improve-should-broadcast-now branch December 12, 2019 12:56
@mfn
Copy link
Contributor

mfn commented Dec 31, 2019

Small PSA:

I'm in the process of upgrading and this broke my tests from Laravel 5.8. Previously broadcast events where collected by the QueueFake but now they are dispatched.

In Laravel 5.8 Queue::fake() works because \Illuminate\Broadcasting\BroadcastManager::queue in fact uses the queue:

        $this->app->make('queue')->connection($connection)->push On(
            $queue, new BroadcastEvent(clone $event)
        );

In the above, make('queue') would return the \Illuminate\Support\Testing\Fakes\QueueFake and thus collect the broadcast event and I can assert on that, instead the event gets fired now.

The solution is to now use the Bus::fake() on such tests and use the assertDispatched thereon.

Note: the same break would be true when upgrading from any Laravel version < 6.7

@driesvints
Copy link
Member

@mfn good catch. But I think it's best that we leave it as is here.

@mfn
Copy link
Contributor

mfn commented Jan 2, 2020

@driesvints yes! Not suggesting a change, just wanted to make something google-worthy for others 😄

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.

6 participants