Skip to content

fix: address critical thread leak and stream corruption bugs#77

Open
ImDevinC wants to merge 1 commit into
mainfrom
fix/critical-discord-rpc-bugs
Open

fix: address critical thread leak and stream corruption bugs#77
ImDevinC wants to merge 1 commit into
mainfrom
fix/critical-discord-rpc-bugs

Conversation

@ImDevinC
Copy link
Copy Markdown
Owner

Summary

This PR addresses critical bugs identified in the Discord RPC plugin that could cause thread accumulation and socket stream corruption, along with several medium-priority improvements.

Critical Fixes

Thread Leak (asyncdiscord.py)

  • Problem: Polling threads were not properly cleaned up on disconnect, causing zombie threads to accumulate with each reconnection
  • Solution: Added thread tracking and proper join with 3s timeout in disconnect()
  • Impact: Prevents memory leaks and thread exhaustion over time

Stream Corruption (sockets.py)

  • Problem: Socket timeout during header read returned None instead of raising an error, causing the stream to become desynchronized
  • Solution: Changed _recv_exact() to always raise EOFError on timeout, removed is_header parameter
  • Impact: Eliminates mysterious disconnections from corrupted stream state

Medium Priority Fixes

Authentication State Checks (backend.py)

  • Problem: API methods only checked connection state, not authentication state, leading to rejected RPC calls
  • Solution: Added _is_authed checks to all 10 RPC API methods
  • Impact: Prevents sending commands before authentication completes

Warning Logs Restored (backend.py)

  • Problem: Silent failures made debugging difficult
  • Solution: Added warning logs for all connection/auth failures
  • Impact: Improved debuggability for users and developers

Connection State Check (asyncdiscord.py)

  • Problem: is_connected() only checked polling flag, not actual socket state
  • Solution: Now checks both self.polling and self.rpc.socket is not None
  • Impact: Catches edge cases where socket died but thread hasn't detected it

Socket Timeout Reduction (constants.py)

  • Problem: 2s timeout caused slow shutdown and compounded thread leak issue
  • Solution: Reduced SOCKET_RECEIVE_TIMEOUT from 2s to 0.5s
  • Impact: Faster shutdown and cleaner reconnects

Additional Improvements

  • Add proper thread safety with _setup_lock for client initialization
  • Remove dead code calling non-existent backend callback methods (DiscordCore.py)
  • Add missing VOICE_SETTINGS_UPDATE event handler (bug fix)
  • Simplify socket implementation from select-based to blocking with timeout
  • Improved error messages and logging consistency

Testing Recommendations

  1. Rapid connect/disconnect cycles (verify no thread accumulation)
  2. Network interruption during send (verify stream recovery)
  3. API calls before authentication (verify warning logs)
  4. Clean shutdown timing (should complete in <1s)
  5. Long-running connection stability

Breaking Changes

None - all changes are internal improvements and bug fixes.

Critical Fixes:
- Thread leak: polling threads now properly joined on disconnect with 3s timeout
- Stream corruption: socket timeouts now raise EOFError instead of returning None
- Connection state: is_connected() now checks actual socket state

Medium Priority Fixes:
- Add authentication state checks to all RPC API methods
- Restore warning logs for better debuggability on connection/auth failures
- Reduce socket timeout from 2s to 0.5s for faster shutdown

Additional Improvements:
- Add proper thread safety with _setup_lock for client initialization
- Remove dead code calling non-existent backend callback methods
- Add missing VOICE_SETTINGS_UPDATE event handler
- Simplify socket implementation from select-based to blocking with timeout

Co-authored-by: opencode <noreply@opencode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant