search

laravel / reverb

Laravel Reverb provides a real-time WebSocket communication backend for Laravel applications.
https://reverb.laravel.com
MIT License
1.05k stars 76 forks source link

Add an activity timeout configuration option #241

Closed zeke0816 closed 3 weeks ago

zeke0816 commented 4 weeks ago

The reason why I am proposing this change is because I actually need this in my own application, where there is one reverb app that I need to ping constantly (every 5 seconds). Plus, I frankly do not understand why there is a ping_interval configuration if it is not used at all except for checking whether you are active or not. In fact, what defines being active is something we should all set for ourselves based on our business rules when developing apps and this rule may vary from application to application, like in my case.

As for breaking existing features, the one thing I would point out is that the default setting in the configurations is 60 seconds, whilst in this branch it was hardcoded to 30 seconds. So, if people were expecting the app to ping every 30 seconds, this should remain unchanged. Therefore, I also changed the configuration file to default to 30 such that the behavior does not change for those who did not set the environment value.

I believe this makes building web applications better because it makes sense out of the configuration and it is an important feature of real-time applications. Being able to change this on an application-basis and quickly via a configuration file and environment variables is a huge advantage for developers, in my honest opinion.

As per tests, I obviously tested this locally and my application indeed started pinging every 5 seconds rather than 30, but I do not really see how to provide a test for this here. This is, after all, my first contribution to any open source repository, and I am not used to it. In any case, I could provide a screenshot showing the results later in comments.

I deeply apologize to everyone if this change is not the intended behavior and I am just talking nonsense, but it really fixes my personal problem and is what I needed. I will understand if this PR is not approved. And, by the way, I would even suggest changing the variable name from $pingInterval to $activityTimeout, since it makes more sense for the app. Even for the Laravel\Reverb\Contracts\Connection::isActive() function.

Thank you for an amazing solution to WebSockets!

joedixon commented 3 weeks ago

I think it makes sense to send a configurable activity_timeout as part of the connection_established event rather than hard coding it. However, I'm not sure it makes sense to reuse ping_interval for this since they have different purposes.

I don't think I'm against adapting this PR to introduce a new activity_timeout config option.

What do you think?

Screenshot 2024-08-16 at 08 47 01
zeke0816 commented 3 weeks ago

I think that's a good idea finding common ground, I was not sure about the usage of ping_interval and I definitely did not find the resource you shared here, so I will update the PR accordingly!

zeke0816 commented 3 weeks ago

Oops, need to pass some tests, I'll fix this now!

taylorotwell commented 3 weeks ago

Drafting this pending review from @joedixon