Bug 14515 - Switching local player to new server (SbS or mySB) loses sync groups
: Switching local player to new server (SbS or mySB) loses sync groups
Status: CLOSED FIXED
Product: SqueezePlay
Classification: Unclassified
Component: Networking
: 7.4.x
: All Other
: P2 normal (vote)
: 7.5.0
Assigned To: Alan Young
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-02 05:19 UTC by Alan Young
Modified: 2010-04-08 17:24 UTC (History)
3 users (show)

See Also:
Category: ---


Attachments
proposed patch (note: untested) (1.34 KB, patch)
2010-02-02 10:35 UTC, Ben Klaas
Details | Diff
updated patch (1.19 KB, patch)
2010-02-04 07:08 UTC, Ben Klaas
Details | Diff
proposed patch #3 (2.19 KB, text/plain)
2010-02-04 09:36 UTC, Alan Young
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Young 2009-10-02 05:19:27 UTC
Because it bypasses the CLI 'connect' command.
Comment 1 Alan Young 2009-11-21 00:24:56 UTC
Tom, this is pretty important for Fab4.
Comment 2 Chris Owens 2009-11-30 09:18:44 UTC
Alan notes that if you're using tinySC and switch to a non-tinySC service (like Pandora) that the other synced players will be left behind.
Comment 3 Chris Owens 2010-01-04 16:00:47 UTC
Changing priorities due to management guidance.
Comment 4 Chris Owens 2010-01-25 18:18:04 UTC
Ben is this something you could take on (assuming we get to P3 bugs)?  If the fix is risky, let's reconsider.
Comment 5 Chris Owens 2010-01-27 15:03:31 UTC
I don't know who this should be assigned to.
Comment 6 Alan Young 2010-01-27 22:24:00 UTC
Ben. And, for my money, this is a pretty important bug.
Comment 7 Ben Klaas 2010-02-02 10:35:29 UTC
Created attachment 6481 [details]
proposed patch (note: untested)

Alan, this is a change to LocalPlayer that will call 'connect' to the CLI if it's currently connected to a server when LocalPlayer:connectToServer() is called

would appreciate your feedback on the patch, particularly in whether to use server:disconnect() or self.slimproto:disconnect() in the else clause...
Comment 8 Alan Young 2010-02-04 00:09:05 UTC
Ben, I would think that the wakeOnLan woudl be appropriate in both cases.

+		--disconnect else serverstatus not being sent (TW: but how to force a serverstatus instead)
+		server:disconnect()
+		-- XXX: Alan, maybe the disconnect should be a slimproto disconnect instead here?
+		-- self.slimproto:disconnect()

No, you cannot send a self.slimproto:disconnect(), otherwise the player will not receive the switch-server command from the existing server, which the point of this change.

But I do not understand what notification you are missing if you do not do server:disconnect(), and what the consequences of that missing notification might be. Ah - I have an idea - it could be that the player-disconnect notification due to the server switch does not happen because player has already been forgotten by the server by the time the disconnect happens, and notifications cannot be sent for forgotten players. Perhaps we ought to delay the forgetClient call but I think that might lead to a timing race with server-status update. I'll think about that some more. I guess the important thing is to force an active comet connection to the new server, so it will get the server-status notifications when the player connects to it.
Comment 9 Ben Klaas 2010-02-04 07:08:37 UTC
Created attachment 6489 [details]
updated patch

sends wakeOnLan packet for both cases

removes server:disconnect() from server switching case

adds log messages for each case
Comment 10 Alan Young 2010-02-04 09:36:53 UTC
Created attachment 6490 [details]
proposed patch #3

The problem was not the lack of disconnect notification, rather that a connect notification was getting lost.
Comment 11 Ben Klaas 2010-02-04 09:53:10 UTC
I'm fine with the additional code to the patch Alan.

What next? Can you give this code a test?
Comment 12 Alan Young 2010-02-04 10:07:01 UTC
I have done some testing and it looks good. I'll do a bunch of testing with various use-cases tomorrow and, if all looks good, I'll check it in.
Comment 13 SVN Bot 2010-02-04 11:10:42 UTC
 == Auto-comment from SVN commit #8454 to the jive repo by ayoung ==
 == https://svn.slimdevices.com/jive?view=revision&revision=8454 ==

bug 14515: Switching local player to new server (SbS or mySB) loses sync groups 
Only use direct connection to local player to change server if we do not have a connection to current server; otherwise use server to initiate switch.
Comment 14 Chris Owens 2010-04-08 17:24:41 UTC
This bug has been marked fixed in a released version of Squeezebox Server or the accompanying firmware or mysqueezebox.com release.

If you are still seeing this issue, please let us know!