Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CLI: fix connection tracking and cleanup for CometD over CLI #1147

Merged
merged 2 commits into from
Aug 18, 2024
Merged

CLI: fix connection tracking and cleanup for CometD over CLI #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
595e636
Cometd.pm: register CLI client with the connection Manager
Aug 18, 2024
d0fdc03
Manager.pm: fix matching CLI connection to clientId
Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Slim/Web/Cometd.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@ sub handler {
elsif ( $obj->{channel} eq '/meta/handshake' ) {
$clid = Slim::Utils::Misc::createUUID();
$manager->add_client( $clid );
if ( $isCLI ) {
$manager->register_connection( $clid, $conn );
}
}
elsif ( $obj->{channel} =~ m{^/slim/(?:subscribe|request)} && $obj->{data} ) {
# Pull clientId out of response channel
Expand Down
2 changes: 1 addition & 1 deletion Slim/Web/Cometd/Manager.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ sub clid_for_connection {
my $result;

while ( my ($clid, $c) = each %{ $self->{conn} } ) {
if ( $conn eq $c ) {
if ( $conn eq $c->[0] ) {
Copy link
Member

Choose a reason for hiding this comment

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

How is this not breaking existing behaviour? Is this another line which previously wasn't hit at all?

Copy link
Author

Choose a reason for hiding this comment

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

Prior to my last PR, the existing CLI <-> CometD was completely broken. The CLI plugin code was fine, it would pass JSON bracketed commands over to CometD but handler in CometD.pm seemed to treat every client as a HTTP client and would fail in multiple spots and never return.

This ended up being sort of a bug because a CLI session would stop working right after the first CometD command never returned a response. You'd start getting BUSY!!!!! responses from the CLI plugin forcing you to disconnect and reconnect to restore function.

My previous PR worked around the trouble spots in handler but I noticed that on client disconnect I would see in the debug log:

[24-08-18 07:39:18.0000] Slim::Web::Cometd::cliCloseHandler (1043) No clid found for CLI connection from 10.0.0.33:48596

So yes this code wasn't getting hit until after my last PR, and then once it started to run, it would never match a socket connection to a clid. The manager would keep the orphaned clids around until server restart. With these changes, the Manager now removes the clid and connection info when the CLI client disconnects:

[24-08-18 07:59:35.3808] Slim::Plugin::CLI::Plugin::client_socket_read (340) Connection with 10.0.0.33:52264 half-closed by peer
[24-08-18 07:59:35.3811] Slim::Plugin::CLI::Plugin::client_socket_close (287) Begin Function
[24-08-18 07:59:35.3817] Slim::Web::Cometd::cliCloseHandler (1029) Lost CLI connection from 10.0.0.33:52264, clid: 54832889
[24-08-18 07:59:35.3819] Slim::Web::Cometd::Manager::remove_connection (62) remove_connection: 54832889
[24-08-18 07:59:35.3824] Slim::Plugin::CLI::Plugin::client_socket_close (305) Closed connection with 10.0.0.33:52264 (0 active connections)
[24-08-18 07:59:45.3833] Slim::Web::Cometd::disconnectClient (1053) Disconnect for 54832889, removing subscriptions
[24-08-18 07:59:45.3838] Slim::Web::Cometd::disconnectClient (1058) Unregistered all auto-execute requests for client 54832889
[24-08-18 07:59:45.3841] Slim::Web::Cometd::Manager::remove_client (93) remove_client: 54832889

$result = $clid;
}
}
Expand Down