Skip to content

Conversation

ashiquzzaman33
Copy link
Contributor

This logic was originally introduced in #251 due to the use of createLazyClient(). Since it was replaced with createClient() in #281, it is no longer needed.

This logic was originally introduced in laravel#251 due to the use of createLazyClient().
Since it was replaced with createClient() in laravel#281, it is no longer needed.
@taylorotwell taylorotwell requested a review from joedixon August 20, 2025 13:38
@taylorotwell taylorotwell marked this pull request as draft August 20, 2025 13:38
@joedixon
Copy link
Collaborator

Thanks @ashiquzzaman33. I understand it's no longer required when using createClient, but is there any reason not to have it in there in case an unsubscribe event is emitted? When using Redis for horizontal scaling, I can't think of a reason why you wouldn't want to ensure the subscription is established.

@ashiquzzaman33
Copy link
Contributor Author

Thanks @joedixon ! 🙌
With createLazyClient() the unsubscribe event was synthetic — emitted automatically when the connection dropped — so auto-resubscribing was needed. With createClient() this isn’t the case: unsubscribe only fires when UNSUBSCRIBE (or closes the client) is explicitly called. Keeping the handler would override intentional unsubs and could even cause loops. For horizontal scaling, #281 already ensures reconnect + resubscribe, so this extra logic isn’t needed.

@ashiquzzaman33
Copy link
Contributor Author

Also in RedisSubscribeClient.php , the subscribe() method doesn’t accept a channel parameter, so the current implementation of RedisPubSubProvider.php would actually throw an error when UNSUBSCRIBE called explicitly.

@joedixon
Copy link
Collaborator

The call to "subscribe" with the channel definitely looks like a bug so we'll get that fixed, but I still wonder about removing the "unsubscribe" listener. In the context of Reverb, when would you want to intentionally unsubscribe? I would argue we want to ensure the connection stays established at all costs.

@ashiquzzaman33
Copy link
Contributor Author

ashiquzzaman33 commented Aug 24, 2025

You're right that we want to keep the connection established, and in the context of Reverb I can’t imagine a case where we’d intentionally unsubscribe. Since we never call unsubscribe, that event will never fire—connection loss triggers close, not unsubscribe, when using createClient. So this listener ends up as dead code. Horizontal scaling is already handled in #281, meaning keeping it adds no value and just leaves unused code.

@ashiquzzaman33
Copy link
Contributor Author

ashiquzzaman33 commented Sep 5, 2025

Hi @joedixon !
Any update on this? Do you still want to keep this dead code?
If that’s the case, I can create a separate PR to fix below bug.

The call to "subscribe" with the channel definitely looks like a bug so we'll get that fixed

Copy link
Collaborator

@joedixon joedixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you this code is no longer necessary. Thanks for the contribution.

@joedixon joedixon marked this pull request as ready for review September 6, 2025 11:51
@taylorotwell taylorotwell merged commit 728f6e2 into laravel:main Sep 7, 2025
19 checks passed
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