fix(music-together): Improve auto-reconnect to fix random disconnections #3688
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the reliability of the music-together plugin by implementing automatic reconnection logic to handle random disconnections that were disrupting user sessions.
Key changes:
- Added automatic reconnection loop that attempts to re-establish connection when network issues occur
- Implemented smarter error handling that distinguishes between recoverable and fatal errors
- Added connection state management with flags to prevent multiple reconnection attempts and respect manual disconnections
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| while (!this.peer.destroyed) { | ||
| if (this.isManualDisconnect) break; | ||
|
|
||
| try { | ||
| if (!this.peer.disconnected) { | ||
| await delay(1000); | ||
| continue; | ||
| } | ||
|
|
||
| console.log( | ||
| 'Music Together: Attempting to reconnect to PeerJS server...', | ||
| ); | ||
| this.peer.reconnect(); | ||
| break; | ||
| } catch (reconnectErr) { | ||
| console.error( | ||
| 'Music Together: Reconnect attempt failed. Retrying in 10 seconds.', | ||
| reconnectErr, | ||
| ); | ||
| // Wait before the next reconnection attempt | ||
| await delay(10000); | ||
| } | ||
| } |
There was a problem hiding this comment.
This infinite loop could run indefinitely if the peer never becomes disconnected or destroyed, potentially consuming CPU resources. Consider adding a maximum retry count or timeout mechanism to prevent endless reconnection attempts.
Copilot uses AI. Check for mistakes.
| this.waitOpen.reject(err); | ||
| this.connectionListeners.forEach((listener) => listener()); | ||
| this.disconnect(); | ||
| this.peer.on('close', () => reconnectLoop()); |
There was a problem hiding this comment.
The 'close' event typically indicates a deliberate disconnection, but this code triggers a reconnection attempt. This could cause unwanted reconnections when the peer is intentionally closed. Consider only triggering reconnectLoop on the 'disconnected' event or adding additional checks.
| this.peer.on('close', () => reconnectLoop()); | |
| // Removed reconnectLoop on 'close' event to avoid unwanted reconnections. |
Copilot uses AI. Check for mistakes.
| console.warn( | ||
| 'Music Together: Disconnected from PeerJS server. The library will attempt to reconnect automatically.', | ||
| ); |
There was a problem hiding this comment.
The 'disconnected' event handler only logs a warning but doesn't trigger any reconnection logic. This inconsistency with the PR's stated goal of automatic reconnection could be confusing. Consider either triggering reconnectLoop here or removing the misleading warning message.
| console.warn( | |
| 'Music Together: Disconnected from PeerJS server. The library will attempt to reconnect automatically.', | |
| ); | |
| reconnectLoop(); |
Copilot uses AI. Check for mistakes.
2 participants
Hello! As someone new to TypeScript, I've done my best to address an issue I was having with the
music-togetherplugin and would appreciate your feedback.Problem/Motivation
I noticed that when using the
music-togetherfeature, the connection would often drop randomly after a period of time (ranging from a few minutes to a few hours). This required manually re-entering the host ID on all connected clients, which was disruptive. The existing error handling didn't seem to account for recoverable network issues, and there wasn't much logging to help diagnose the problem.Solution
I've modified the
connection.tsfile to build a more resilient connection handling system. The key changes are:reconnectLoopthat automatically tries to re-establish a connection to the PeerJS server if it's lost unexpectedly.peer.on('error', ...)handler now distinguishes between recoverable network errors (likePeerUnavailableorServerError) and fatal errors. It will only attempt to reconnect on recoverable errors.isManualDisconnectandisReconnectingflags to ensure the reconnection logic only runs when it should (i.e., not after a user manually disconnects) and to prevent multiple loops from running at once.console.logandconsole.warnstatements to provide clear feedback on the connection status, disconnection reasons, and reconnection attempts. This should make future troubleshooting much easier.Testing
music-togethersession and left it running for several hours, observing that it successfully recovered from network fluctuations without requiring a manual reconnect.Thank you for considering my contribution!