|
| 1 | +# SignalR Web Platform Refactoring |
| 2 | + |
| 3 | +## Summary |
| 4 | + |
| 5 | +This refactoring addresses multiple issues with the SignalR infrastructure to ensure correct operation on the web platform while preventing memory leaks, threading issues, and connection starvation. |
| 6 | + |
| 7 | +## Issues Addressed |
| 8 | + |
| 9 | +### 1. Memory Leaks |
| 10 | + |
| 11 | +**Problem:** Event listeners were registered via `signalRService.on()` in the SignalR store but never removed with `signalRService.off()` on disconnect. This caused: |
| 12 | +- Accumulation of duplicate event handlers on reconnection |
| 13 | +- Memory growth over time |
| 14 | +- Multiple callbacks firing for single events |
| 15 | + |
| 16 | +**Solution:** |
| 17 | +- Added `EventHandlers` interface to track registered handlers in `signalr-store.ts` |
| 18 | +- Created `unregisterUpdateHubHandlers()` function for proper cleanup |
| 19 | +- Handlers are now stored as named references that can be properly unregistered |
| 20 | +- Cleanup happens automatically on disconnect |
| 21 | + |
| 22 | +### 2. Reconnection Timeout Leaks |
| 23 | + |
| 24 | +**Problem:** `setTimeout` IDs for reconnection attempts were not tracked, making them impossible to cancel properly. This caused: |
| 25 | +- Multiple concurrent reconnection attempts |
| 26 | +- Resource waste when intentionally disconnecting |
| 27 | +- Orphaned timers continuing to fire after connection was established |
| 28 | + |
| 29 | +**Solution:** |
| 30 | +- Added `reconnectTimeouts` Map to track timeout IDs per hub |
| 31 | +- Added `cancelPendingReconnect()` and `cancelAllPendingReconnects()` methods |
| 32 | +- Timeouts are cancelled on: |
| 33 | + - Successful connection |
| 34 | + - Explicit disconnect |
| 35 | + - Service reset |
| 36 | + - Page visibility change (web) |
| 37 | + |
| 38 | +### 3. Web Platform Visibility Handling |
| 39 | + |
| 40 | +**Problem:** Reconnection timers would fire even when the browser tab was backgrounded, wasting resources and potentially causing issues with backgrounded tabs. |
| 41 | + |
| 42 | +**Solution:** |
| 43 | +- Added visibility change event listener using `document.visibilityState` |
| 44 | +- Reconnection attempts are skipped when page is not visible |
| 45 | +- On visibility resume, connections are checked and reconnected if needed |
| 46 | +- Reconnect attempts counter is reset on visibility resume for fresh attempts |
| 47 | +- Proper cleanup of visibility listener on service destruction |
| 48 | + |
| 49 | +### 4. Connection Cancellation |
| 50 | + |
| 51 | +**Problem:** No way to cancel pending connection attempts, leading to: |
| 52 | +- Stuck connection states |
| 53 | +- Multiple overlapping connection attempts |
| 54 | +- Difficulty in clean disconnect |
| 55 | + |
| 56 | +**Solution:** |
| 57 | +- Added `AbortController` for each connection attempt |
| 58 | +- Previous pending connections are cancelled when new connection is initiated |
| 59 | +- Cancellation is handled gracefully without error logging |
| 60 | +- Pending connections are cancelled on explicit disconnect |
| 61 | + |
| 62 | +### 5. Exponential Backoff for Reconnection |
| 63 | + |
| 64 | +**Problem:** Fixed reconnection interval could overwhelm servers during outages. |
| 65 | + |
| 66 | +**Solution:** |
| 67 | +- Implemented exponential backoff with `RECONNECT_BACKOFF_MULTIPLIER = 1.5` |
| 68 | +- Maximum delay capped at 30 seconds |
| 69 | +- Provides better server load distribution during issues |
| 70 | + |
| 71 | +### 6. Hub Method Handler Cleanup |
| 72 | + |
| 73 | +**Problem:** Method handlers registered on the SignalR connection were not being tracked or cleaned up, leading to duplicate handlers. |
| 74 | + |
| 75 | +**Solution:** |
| 76 | +- Added `hubMethodHandlers` Map to track handlers per hub |
| 77 | +- Created `cleanupHubMethodHandlers()` method for proper cleanup |
| 78 | +- Handlers are unregistered on: |
| 79 | + - Connection disconnect |
| 80 | + - Reconnection (before new registration) |
| 81 | + - Service reset |
| 82 | + |
| 83 | +## Code Changes |
| 84 | + |
| 85 | +### `src/services/signalr.service.ts` |
| 86 | + |
| 87 | +1. **New Properties:** |
| 88 | + ```typescript |
| 89 | + private reconnectTimeouts: Map<string, ReturnType<typeof setTimeout>> = new Map(); |
| 90 | + private hubMethodHandlers: Map<string, HubMethodHandler[]> = new Map(); |
| 91 | + private isPageVisible: boolean = true; |
| 92 | + private visibilityChangeHandler: (() => void) | null = null; |
| 93 | + private pendingConnections: Map<string, AbortController> = new Map(); |
| 94 | + private readonly RECONNECT_BACKOFF_MULTIPLIER = 1.5; |
| 95 | + ``` |
| 96 | + |
| 97 | +2. **New Methods:** |
| 98 | + - `setupVisibilityHandling()` - Sets up web visibility change listener |
| 99 | + - `cleanupVisibilityHandling()` - Removes visibility listener |
| 100 | + - `cancelAllPendingReconnects()` - Cancels all pending reconnection timeouts |
| 101 | + - `cancelPendingReconnect(hubName)` - Cancels specific hub reconnection timeout |
| 102 | + - `checkAndReconnectOnVisibilityResume()` - Reconnects disconnected hubs when tab becomes visible |
| 103 | + - `cleanupHubMethodHandlers(hubName, connection)` - Removes registered method handlers |
| 104 | + - `offAll(event)` - Removes all listeners for a specific event |
| 105 | + - `removeAllListeners()` - Removes all event listeners |
| 106 | + - `getEventListenerCount(event)` - Debug utility to count listeners |
| 107 | + - `getTotalEventListenerCount()` - Debug utility for total listener count |
| 108 | + - `isVisible()` - Returns current page visibility state |
| 109 | + |
| 110 | +3. **Modified Methods:** |
| 111 | + - `_connectToHubWithEventingUrlInternal()` - Added AbortController, method handler tracking |
| 112 | + - `_connectToHubInternal()` - Added AbortController, method handler tracking |
| 113 | + - `handleConnectionClose()` - Added timeout tracking, backoff delay, visibility check |
| 114 | + - `disconnectFromHub()` - Added cleanup for timeouts and pending connections |
| 115 | + - `resetInstance()` - Added comprehensive cleanup |
| 116 | + - `disconnectAll()` - Added cleanup for pending operations |
| 117 | + |
| 118 | +### `src/stores/signalr/signalr-store.ts` |
| 119 | + |
| 120 | +1. **New Module-Level Code:** |
| 121 | + ```typescript |
| 122 | + interface EventHandlers { |
| 123 | + personnelStatusUpdated: ((data: unknown) => void) | null; |
| 124 | + // ... other handlers |
| 125 | + } |
| 126 | + |
| 127 | + let updateHubHandlers: EventHandlers = { /* ... */ }; |
| 128 | + |
| 129 | + function unregisterUpdateHubHandlers(): void { /* ... */ } |
| 130 | + ``` |
| 131 | + |
| 132 | +2. **Modified `connectUpdateHub`:** |
| 133 | + - Calls `unregisterUpdateHubHandlers()` before registering new handlers |
| 134 | + - Stores handler references for later cleanup |
| 135 | + - Logs listener count for debugging |
| 136 | + |
| 137 | +3. **Modified `disconnectUpdateHub`:** |
| 138 | + - Calls `unregisterUpdateHubHandlers()` before disconnecting |
| 139 | + - Logs remaining listener count for debugging |
| 140 | + |
| 141 | +## Testing |
| 142 | + |
| 143 | +All existing tests continue to pass: |
| 144 | +- `src/services/__tests__/signalr.service.test.ts` - Core service tests |
| 145 | +- `src/services/__tests__/signalr.service.reconnect-fix.test.ts` - Reconnection tests |
| 146 | +- `src/stores/signalr/__tests__/signalr-store.test.ts` - Store tests |
| 147 | + |
| 148 | +## Migration Notes |
| 149 | + |
| 150 | +- No breaking changes to the public API |
| 151 | +- Existing code using `signalRService.on()` should continue to work |
| 152 | +- For best practices, use the new `getTotalEventListenerCount()` method to monitor for leaks during development |
| 153 | +- The visibility handling is automatic on web platform |
| 154 | + |
| 155 | +## Debugging Memory Leaks |
| 156 | + |
| 157 | +If you suspect memory leaks, you can check listener counts: |
| 158 | + |
| 159 | +```typescript |
| 160 | +// Check total listeners |
| 161 | +console.log('Total listeners:', signalRService.getTotalEventListenerCount()); |
| 162 | + |
| 163 | +// Check specific event listeners |
| 164 | +console.log('callsUpdated listeners:', signalRService.getEventListenerCount('callsUpdated')); |
| 165 | +``` |
| 166 | + |
| 167 | +Expected values: |
| 168 | +- After connect: 7 listeners (one per event type) |
| 169 | +- After disconnect: 0 listeners |
| 170 | +- Multiple connect/disconnect cycles should maintain these numbers |
0 commit comments