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

When using scaling = true the toOthers method in Laravel does not work #246

Closed
gofish543 opened this issue Sep 4, 2024 · 6 comments
Closed
Assignees

Comments

@gofish543
Copy link

Reverb Version

1..0

Laravel Version

11.21

PHP Version

8.3

Description

In the code block below the $connection variable of the dispatch function is ignored when using the PubSubProvider->publish(...). This is more of a question than a bug report, why is this the case and is there a way to get the PubSubProvider to properly ignore the passed $connection?

<?php

namespace Laravel\Reverb\Protocols\Pusher;

use Illuminate\Support\Arr;
use Laravel\Reverb\Application;
use Laravel\Reverb\Contracts\Connection;
use Laravel\Reverb\Protocols\Pusher\Contracts\ChannelManager;
use Laravel\Reverb\ServerProviderManager;
use Laravel\Reverb\Servers\Reverb\Contracts\PubSubProvider;

class EventDispatcher
{
    /**
     * Dispatch a message to a channel.
     */
    public static function dispatch(Application $app, array $payload, ?Connection $connection = null): void
    {
        $server = app(ServerProviderManager::class);

        if ($server->shouldNotPublishEvents()) {
            static::dispatchSynchronously($app, $payload, $connection);

            return;
        }
        
        // Enabled by `scaling = true` and does not take the $connectiton variable and therefore ignores `toOthers()`
        app(PubSubProvider::class)->publish([
            'type' => 'message',
            'application' => serialize($app),
            'payload' => $payload,
        ]);
    }

    /**
     * Notify all connections subscribed to the given channel.
     */
    public static function dispatchSynchronously(Application $app, array $payload, ?Connection $connection = null): void
    {
        $channels = Arr::wrap($payload['channels'] ?? $payload['channel'] ?? []);

        foreach ($channels as $channel) {
            unset($payload['channels']);

            if (! $channel = app(ChannelManager::class)->for($app)->find($channel)) {
                continue;
            }

            $payload['channel'] = $channel->name();

            $channel->broadcast($payload, $connection);
        }
    }
}

Steps To Reproduce

  1. Create a Laravel app with a Broadcasting event
  2. Broadcast the even toOthers
  3. Ensure reverb is set to scaling = true
  4. Observe toOthers is ignored by reverb server
@joedixon
Copy link
Collaborator

joedixon commented Sep 9, 2024

I'll take a look at this soon.

@gofish543
Copy link
Author

I'll take a look at this soon.

It's really just a technical question relating to the PubSubProvider::class and if it's implementation / use in scaling mode can even support toOthers. At the end of day, it causes an extra round trip request form my client application and isn't critical... more of a micro optimization / papercut :)

@samuelrac
Copy link

samuelrac commented Nov 7, 2024

I have same issue, when scaling = true, toOthers not work.

@MarcelM1109
Copy link

It appears that the error also occurs when using the Whisper client event.

@larswolff
Copy link
Contributor

I hit this problem and have created a PR that fixes the issue in my installation:

#275

taylorotwell added a commit that referenced this issue Nov 27, 2024
* Fix for issue #246

Pass SocketID through redis, and filter it on the receiving end.

This ensures that ->toOthers() works with scaling = true

* update tests

* formatting

* wip

* Update PusherPubSubIncomingMessageHandler.php

---------

Co-authored-by: Joe Dixon <[email protected]>
Co-authored-by: Taylor Otwell <[email protected]>
@joedixon
Copy link
Collaborator

joedixon commented Dec 1, 2024

Thanks @larswolff that's now been merged. Closing this issue now.

@joedixon joedixon closed this as completed Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants