Add support for server heartbeats #39
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
alberts-s
force-pushed
the
implement-server-pings
branch
from
c8a3227
to
7e53f8a
Compare
|
Thank you for your effort! I agree that server side pings would be beneficial for clients that don't support sending out heartbeats. If the PR is ready to merge, please add a line to the changelog.md that describes your change. |
Before this change the websocket server would not have any heartbeat mechanism, this meant that unless the client proactively implements heartbeats the server would close the connection. From what I have seen most non-browser clients do not implement heartbeats (e.g filebeat, websocat), thus for those clients the connection would have been closed in 65 seconds. The Websocket RFC does not set out a requirement which side should initiate heartbeats or that they are required. Most browsers have implemented heartbeats from client side, however it is not a must thus, in my opinion it is beneficial for the server to implement them, especially if the server closes connection if heartbeat is not received. As a consequence this changeset allows to support clients which aren't browsers.
alberts-s
force-pushed
the
implement-server-pings
branch
from
0012acd
to
41bc270
Compare
alberts-s
changed the title
|
Hi @d-Rickyy-b, |
While this is great for debugging purposes, this rather spams the log with a large number of clients.
I ignored them before, and even though it's unlikely for errors to occur here, It's better to handle them properly.
|
Hi @alberts-s and sorry for the late response. I had plenty of things going on and only now am able to properly review and test your changes. Despite the changes not being very huge, I still want to make sure everything is working properly. Thank you for your contribution! |
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.
Before this change the websocket server would not have any heartbeat mechanism, this meant that unless the client proactively implements heartbeats the server would close the connection. From what I have seen most non-browser clients do not implement heartbeats (e.g filebeat, websocat), thus for those clients the connection would have been closed in 65 seconds.
The Websocket RFC does not set out a requirement which side should initiate heartbeats or that they are required. Most browsers have implemented heartbeats from client side, however it is not a must thus, in my opinion it is beneficial for the server to implement them, especially if the server closes connection if heartbeat is not received. This would allow to support clients which aren't browsers.