[Slim-Checkins] r33928 - in /7.8/trunk/server/Slim: Control/Queries.pm Web/Cometd.pm

Author: ayoung
Date: Wed Apr  4 08:34:41 2012
New Revision: 33928

URL: http://svn.slimdevices.com/slim?rev=33928&view=rev
Log:
Resolve a couple of problems player-status subscription reliability. 

Need to extract clientId from /slim/unsubscribe Comet requests. This was one of the causes of SP clients not always reconnecting "completely". If the first request in a packet was an unsubscribe then the whole packey would be dropped.

Do not remove the repeating subscription from a (player)status query when the client has gone as otherwise a Comet subscription can be dropped.

Do not give up processing requests from a Comet packet upon any error, other than conditions that explicitly do this.

Stop processing requests from a Comet packet after getting a subscription request that results in an errorNeedClient condition.

Remove duplicate checking for valid clientId.

Modified:
    7.8/trunk/server/Slim/Control/Queries.pm
    7.8/trunk/server/Slim/Web/Cometd.pm

Modified: 7.8/trunk/server/Slim/Control/Queries.pm
URL: http://svn.slimdevices.com/slim/7.8/trunk/server/Slim/Control/Queries.pm?rev=33928&r1=33927&r2=33928&view=diff
==============================================================================
--- 7.8/trunk/server/Slim/Control/Queries.pm (original)
+++ 7.8/trunk/server/Slim/Control/Queries.pm Wed Apr  4 08:34:41 2012
@@ -3004,9 +3004,10 @@
 	# accomodate the fact we can be called automatically when the client is gone
 	if (!defined($client)) {
 		$request->addResult('error', "invalid player");
-		$request->registerAutoExecute('-');
-		$request->setStatusDone();
-		return;
+		# Still need to (re)register the autoexec if this is a subscription so
+		# that the subscription does not dissappear while a Comet client thinks
+		# that it is still valid.
+		goto do_it_again;
 	}
 	
 	my $connected    = $client->connected() || 0;
@@ -3404,7 +3405,7 @@
 		}
 	}
 
-
+do_it_again:
 	# manage the subscription
 	if (defined(my $timeout = $request->getParam('subscribe'))) {
 		main::DEBUGLOG && $isDebug && $log->debug("statusQuery(): setting up subscription");

Modified: 7.8/trunk/server/Slim/Web/Cometd.pm
URL: http://svn.slimdevices.com/slim/7.8/trunk/server/Slim/Web/Cometd.pm?rev=33928&r1=33927&r2=33928&view=diff
==============================================================================
--- 7.8/trunk/server/Slim/Web/Cometd.pm (original)
+++ 7.8/trunk/server/Slim/Web/Cometd.pm Wed Apr  4 08:34:41 2012
@@ -176,6 +176,10 @@
 				# Pull clientId out of response channel
 				($clid) = $obj->{data}->{response} =~ m{/([0-9a-f]{8})/};
 			}
+			elsif ( $obj->{channel} =~ m{^/slim/unsubscribe} && $obj->{data} ) {
+				# Pull clientId out of unsubscribe
+				($clid) = $obj->{data}->{unsubscribe} =~ m{/([0-9a-f]{8})/};
+			}
 			
 			# Register client with HTTP connection
 			if ( $clid ) {
@@ -189,10 +193,14 @@
 					error   => 'No clientId found',
 					id      => $obj->{id},
 				};
-			}
-		}
-		
-		last if @errors;
+				
+				# No point in trying to process requests without a clientId
+				# Given that we are not sending advice to the client, the pending
+				# requests may just get dropped but what can we do?
+				$log->error('Invalid request without clientId - discarding remaining requests in packet');
+				last;
+			}
+		}
 		
 		# Detect the language Jive wants content returned in
 		my ($lang, $ua);
@@ -325,76 +333,42 @@
 		}
 		elsif ( $obj->{channel} eq '/meta/subscribe' ) {
 			
-			if ( !$manager->is_valid_clid( $clid ) ) {
-				# Invalid clientId, send advice to re-handshake
-				
+			my $subscriptions = $obj->{subscription};
+		
+			# a channel name or a channel pattern or an array of channel names and channel patterns.
+			if ( !ref $subscriptions ) {
+				$subscriptions = [ $subscriptions ];
+			}
+		
+			$manager->add_channels( $clid, $subscriptions );
+		
+			for my $sub ( @{$subscriptions} ) {
 				push @{$events}, {
-					channel    => '/meta/subscribe',
-					clientId   => undef,
-					successful => JSON::XS::false,
-					timestamp  => time2str( time() ),
-					error      => 'invalid clientId',
-					advice     => {
-						reconnect => 'handshake',
-						interval  => 0,
-					}
+					channel      => '/meta/subscribe',
+					clientId     => $clid,
+					successful   => JSON::XS::true,
+					subscription => $sub,
 				};
 			}
-			else {
-				my $subscriptions = $obj->{subscription};
-			
-				# a channel name or a channel pattern or an array of channel names and channel patterns.
-				if ( !ref $subscriptions ) {
-					$subscriptions = [ $subscriptions ];
-				}
-			
-				$manager->add_channels( $clid, $subscriptions );
-			
-				for my $sub ( @{$subscriptions} ) {
-					push @{$events}, {
-						channel      => '/meta/subscribe',
-						clientId     => $clid,
-						successful   => JSON::XS::true,
-						subscription => $sub,
-					};
-				}
-			}
 		}
 		elsif ( $obj->{channel} eq '/meta/unsubscribe' ) {
 			
-			if ( !$manager->is_valid_clid( $clid ) ) {
-				# Invalid clientId, send advice to re-handshake
-				
+			my $subscriptions = $obj->{subscription};
+		
+			# a channel name or a channel pattern or an array of channel names and channel patterns.
+			if ( !ref $subscriptions ) {
+				$subscriptions = [ $subscriptions ];
+			}
+		
+			$manager->remove_channels( $clid, $subscriptions );
+		
+			for my $sub ( @{$subscriptions} ) {
 				push @{$events}, {
-					channel    => '/meta/unsubscribe',
-					clientId   => undef,
-					successful => JSON::XS::false,
-					timestamp  => time2str( time() ),
-					error      => 'invalid clientId',
-					advice     => {
-						reconnect => 'handshake',
-						interval  => 0,
-					}
+					channel      => '/meta/unsubscribe',
+					clientId     => $clid,
+					subscription => $sub,
+					successful   => JSON::XS::true,
 				};
-			}
-			else {
-				my $subscriptions = $obj->{subscription};
-			
-				# a channel name or a channel pattern or an array of channel names and channel patterns.
-				if ( !ref $subscriptions ) {
-					$subscriptions = [ $subscriptions ];
-				}
-			
-				$manager->remove_channels( $clid, $subscriptions );
-			
-				for my $sub ( @{$subscriptions} ) {
-					push @{$events}, {
-						channel      => '/meta/unsubscribe',
-						clientId     => $clid,
-						subscription => $sub,
-						successful   => JSON::XS::true,
-					};
-				}
 			}
 		}
 		elsif ( $obj->{channel} eq '/slim/subscribe' ) {
@@ -441,7 +415,10 @@
 						id      => $id,
 					};
 					
+					push @errors, \%error;
+
 					if ($result->{'errorNeedClient'}) {
+						$log->error('errorNeedsClient: ', join(', ', $request->[0], @{$request->[1]}));
 						# Force reconnect because client not connected.
 						# We should not need to force a new handshake, just a reconect.
 						# Any successful subscribes will have the acknowledgements in the $events queue
@@ -450,9 +427,12 @@
 						$error{'advice'} = {
 							reconnect => 'retry',
 						};
+						
+						# The client will stop processing responses after this error with reconnect advice
+						# so stop processing further requests.
+						last;
 					}
 						
-					push @errors, \%error;
 				}
 				else {
 					push @{$events}, {
@@ -930,7 +910,7 @@
 sub requestCallback {
 	my $request = shift;
 	
-	my ($channel, $id, $priority, $clid) = split /\|/, $request->source, 4;
+	my ($channel, $id, $priority, $clid) = split (/\|/, $request->source, 4);
 	
 	main::DEBUGLOG && $log->debug( "requestCallback got results for $channel / $id" );