use keepalive on CLI #629
Merged
use keepalive on CLI #629
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
Instead of the more aggressive closing of all sockets when a peer disconnects, use TCP keepalive on cli sockets + a monitoring timer. The reason is that multiple instances of an application can share the same target IP address and we don't want to aggressively force close all CLI active socket just because a single instance failed
michaelherger
requested changes
Jun 30, 2021
Slim/Plugin/CLI/Plugin.pm
Outdated
| @@ -278,63 +281,62 @@ sub cli_socket_accept { | |||
| } | |||
| } | |||
|
|
|||
| sub client_socket_keepalive { | |||
| my $client_socket = shift; | |||
|
|
|||
There was a problem hiding this comment.
Please add a killTimers() call here, too. We've had problems in the past where insane numbers of timers got created for whatever reason. Better be safe than sorry.
There was a problem hiding this comment.
I can also not do the timer thing at all and see how it goes. I've tested that and when the KEEPALIVE kicks-in (the peer has disappeared), the socket is closed (at least on Windows). So we can see what happens.
This reverts commit 2fd9aeb.
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.
Instead of the more aggressive closing of all sockets when a peer disconnects, use TCP keepalive on cli sockets + a monitoring timer. The reason is that multiple instances of an application can share the same target IP address and we don't want to aggressively force close all CLI active socket just because a single instance failed