Add an activity timeout configuration option #241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current `activity_timeout` option when establishing a connection was hardcoded to 30 seconds, but sometimes we need our application to ping at a different rate. This value should be configured through the proper config file using the `ping_interval` option which takes `REVERB_APP_PING_INTERVAL` environment variable or 60 by default. This update takes the value which is loaded onto the `Application` class and is available through the `Connection` class, making this an easy fix.
This change makes sure that all future applications that re-publish the configuration keep the original 30 second value instead of the 60 second default configuration. All applications that modified this value in their environment file should not be affected.
|
I think it makes sense to send a configurable I don't think I'm against adapting this PR to introduce a new What do you think? 
|
|
I think that's a good idea finding common ground, I was not sure about the usage of |
…stead of `ping_interval` to separate the concerns and accomplish the same result.
|
Oops, need to pass some tests, I'll fix this now! |
…finition. Using `30` as the default value as this was the original value in the package.
zeke0816
changed the title
zeke0816
changed the title
|
Drafting this pending review from @joedixon |
joedixon
approved these changes
Aug 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_intervalconfiguration 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
60seconds, whilst in this branch it was hardcoded to30seconds. So, if people were expecting the app to ping every30seconds, this should remain unchanged. Therefore, I also changed the configuration file to default to30such 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
$pingIntervalto$activityTimeout, since it makes more sense for the app. Even for theLaravel\Reverb\Contracts\Connection::isActive()function.Thank you for an amazing solution to WebSockets!