mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
fix(#148) android root cause: single-socket-per-device invariant (no duplicate connections)
The player opened duplicate/rapid WebSocket connections for the same device_id: connect() was unconditional (disconnect + forceNew socket) and reachable from every lifecycle entry point (boot, service start, MainActivity/ProvisioningActivity bind, foreground re-bind, START_STICKY). A ROM that re-binds on foreground (MAXHUB PROC_STATE_TOP, isBindService:true) therefore re-invoked connect() repeatedly -> a burst of sockets, each evicted by the next (the 8-in-9s storm). Fire TV never re-binds like that, so it never reproduced. - ConnectionGuard (new, pure/testable — service is the shell, per the OtaThrottle pattern): shouldOpenNewSocket(hasSocket, sameUrl, socketActive) — reuse a live/self-healing socket to the same url; open a new one only when none is usable. - WebSocketService: connect() is now idempotent (@Synchronized + ConnectionGuard) — every entry point reuses the one socket, never opens a duplicate; body split into openSocket(). socketActive / currentUrl track the single socket. - Single owner: onStartCommand now calls connect() so the SERVICE owns the one connection (idempotent across START_STICKY restarts), not whichever activity binds. - Reconnect discipline: on io server/client disconnect (which Socket.IO does NOT auto-reconnect) mark the socket inert and schedule exactly ONE backed-off re-open — never a blind re-open loop; a transport drop keeps socketActive=true so Socket.IO's own reconnect is reused. Test: ConnectionGuardTest (5, incl. 8-rapid-binds-all-reuse). :app:testDebugUnitTest green (ConnectionGuard 5, OtaThrottle 7, ScheduleEval 1). NOT bumped/signed/released — Dan builds+signs with the BMG keystore; 1.9.2-patch2 (server net) covers un-updated devices.
This commit is contained in:
parent
bd5f4253ae
commit
1a5c468537
|
|
@ -0,0 +1,29 @@
|
|||
package com.remotedisplay.player.service
|
||||
|
||||
/**
|
||||
* #148 root-cause guard: the SINGLE-SOCKET-PER-DEVICE invariant, extracted from
|
||||
* [WebSocketService] so it is unit-testable without a live Socket.IO / Android (the service is
|
||||
* just the shell — same pattern as OtaThrottle). It decides whether a connect() request should
|
||||
* OPEN a new socket or REUSE the one already held.
|
||||
*
|
||||
* WHY (#148): every entry point — BOOT_COMPLETED -> service start, Activity bind
|
||||
* (onServiceConnected), a foreground re-bind (the MAXHUB PROC_STATE_TOP isBindService:true
|
||||
* transition), Socket.IO reconnect — used to call an UNCONDITIONAL connect() that tore down any
|
||||
* healthy socket and opened a forceNew one. A ROM that re-binds on foreground therefore produced
|
||||
* a burst of sockets for the same device_id (the 8-in-9s storm). Fire TV never re-binds like
|
||||
* that, so it never reproduced. This guards the SOCKET (the thing that matters), not the
|
||||
* service/bind count (the thing that varies), so it closes every duplication vector at once.
|
||||
*/
|
||||
object ConnectionGuard {
|
||||
/**
|
||||
* Should connect(url) open a NEW socket, or reuse the current one?
|
||||
*
|
||||
* Reuse (return false) iff we already hold a socket to the SAME url that is live or
|
||||
* self-healing — [socketActive] means connected OR Socket.IO is auto-reconnecting it. Open a
|
||||
* new one (return true) only when there is none usable: no socket, a different url (a genuine
|
||||
* re-provision to another server), or the socket went inert (e.g. after `io server
|
||||
* disconnect`, which Socket.IO does not auto-reconnect).
|
||||
*/
|
||||
fun shouldOpenNewSocket(hasSocket: Boolean, sameUrl: Boolean, socketActive: Boolean): Boolean =
|
||||
!(hasSocket && sameUrl && socketActive)
|
||||
}
|
||||
|
|
@ -22,12 +22,25 @@ import java.net.URI
|
|||
class WebSocketService : Service() {
|
||||
|
||||
private var socket: Socket? = null
|
||||
// #148 root-cause guard: the single-socket invariant. currentUrl + socketActive track the
|
||||
// ONE socket so connect() is idempotent across every entry point (boot, service start,
|
||||
// activity bind, foreground re-bind) — a re-bind can never open a duplicate. See
|
||||
// ConnectionGuard. socketActive == "connected OR Socket.IO is auto-reconnecting it".
|
||||
@Volatile private var socketActive = false
|
||||
@Volatile private var currentUrl: String? = null
|
||||
private var reopenRunnable: Runnable? = null
|
||||
private lateinit var config: ServerConfig
|
||||
private lateinit var deviceInfo: DeviceInfo
|
||||
private val handler = Handler(Looper.getMainLooper())
|
||||
private var heartbeatRunnable: Runnable? = null
|
||||
private val binder = LocalBinder()
|
||||
|
||||
companion object {
|
||||
// #148: backoff before re-opening the single socket after a disconnect that Socket.IO
|
||||
// does NOT auto-reconnect (io server/client disconnect) — never a blind immediate re-open.
|
||||
private const val RECONNECT_AFTER_EVICT_MS = 3000L
|
||||
}
|
||||
|
||||
// Callbacks
|
||||
var onPaired: ((String, String) -> Unit)? = null
|
||||
var onUnpaired: (() -> Unit)? = null
|
||||
|
|
@ -76,6 +89,10 @@ class WebSocketService : Service() {
|
|||
}
|
||||
|
||||
override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
|
||||
// #148 single owner: the SERVICE owns the one connection. Idempotent (ConnectionGuard),
|
||||
// so a START_STICKY restart / re-delivery / boot start reuses a live socket and never
|
||||
// opens a duplicate. No-op until a server url is configured (provisioning).
|
||||
connect()
|
||||
return START_STICKY
|
||||
}
|
||||
|
||||
|
|
@ -94,14 +111,31 @@ class WebSocketService : Service() {
|
|||
return this
|
||||
}
|
||||
|
||||
/**
|
||||
* Idempotent connect — the #148 root-cause guard. Safe to call from EVERY entry point
|
||||
* (service start, activity bind, foreground transition, reconnect). If we already hold a
|
||||
* live or self-healing socket to the same url, REUSE it; only ever open ONE socket per
|
||||
* device. @Synchronized so racing entry points can't open two.
|
||||
*/
|
||||
@Synchronized
|
||||
fun connect(serverUrl: String? = null) {
|
||||
val url = serverUrl ?: config.serverUrl
|
||||
if (url.isEmpty()) {
|
||||
Log.e("WebSocketService", "No server URL configured")
|
||||
return
|
||||
}
|
||||
if (!ConnectionGuard.shouldOpenNewSocket(socket != null, currentUrl == url, socketActive)) {
|
||||
Log.i("WebSocketService", "connect(): reusing existing socket to $url — no duplicate (#148)")
|
||||
return
|
||||
}
|
||||
openSocket(url)
|
||||
}
|
||||
|
||||
@Synchronized
|
||||
private fun openSocket(url: String) {
|
||||
disconnect()
|
||||
currentUrl = url
|
||||
socketActive = true
|
||||
|
||||
try {
|
||||
val options = IO.Options().apply {
|
||||
|
|
@ -126,8 +160,16 @@ class WebSocketService : Service() {
|
|||
val reason = args.firstOrNull()?.toString() ?: "unknown"
|
||||
Log.w("WebSocketService", "Disconnected from server: $reason")
|
||||
// Stop heartbeat while disconnected; player keeps showing cached content.
|
||||
// Socket.IO will reconnect automatically per the options above.
|
||||
stopHeartbeat()
|
||||
// #148 reconnect discipline: Socket.IO auto-reconnects the SAME socket on a
|
||||
// transport drop (reconnection=true) — leave socketActive true so connect()
|
||||
// keeps reusing it. But on a server- or client-initiated disconnect it does
|
||||
// NOT auto-reconnect, so mark the socket inert and bring up exactly ONE new
|
||||
// connection after a backoff — never a blind re-open that gets evicted again.
|
||||
if (reason == "io server disconnect" || reason == "io client disconnect") {
|
||||
socketActive = false
|
||||
scheduleReopen(RECONNECT_AFTER_EVICT_MS)
|
||||
}
|
||||
}
|
||||
|
||||
safeOn(Socket.EVENT_CONNECT_ERROR) { args ->
|
||||
|
|
@ -613,11 +655,28 @@ class WebSocketService : Service() {
|
|||
|
||||
fun disconnect() {
|
||||
stopHeartbeat()
|
||||
cancelReopen()
|
||||
socketActive = false
|
||||
try { socket?.disconnect() } catch (e: Throwable) { Log.w("WebSocketService", "disconnect: ${e.message}") }
|
||||
try { socket?.off() } catch (e: Throwable) { Log.w("WebSocketService", "off: ${e.message}") }
|
||||
socket = null
|
||||
}
|
||||
|
||||
// #148 reconnect discipline: bring up exactly ONE new connection after an eviction, with a
|
||||
// backoff, and only one pending at a time — so a server eviction can't trigger a blind
|
||||
// re-open loop. connect() itself is idempotent, so this is safe even if an activity rebind
|
||||
// races it.
|
||||
private fun scheduleReopen(delayMs: Long) {
|
||||
if (reopenRunnable != null) return
|
||||
val r = Runnable { reopenRunnable = null; connect() }
|
||||
reopenRunnable = r
|
||||
handler.postDelayed(r, delayMs)
|
||||
}
|
||||
private fun cancelReopen() {
|
||||
reopenRunnable?.let { handler.removeCallbacks(it) }
|
||||
reopenRunnable = null
|
||||
}
|
||||
|
||||
fun isConnected(): Boolean = socket?.connected() == true
|
||||
|
||||
override fun onDestroy() {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,47 @@
|
|||
package com.remotedisplay.player.service
|
||||
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* #148: the single-socket-per-device invariant (root-cause fix for the client opening
|
||||
* duplicate/rapid sockets). ConnectionGuard is the testable core; WebSocketService is the shell.
|
||||
*/
|
||||
class ConnectionGuardTest {
|
||||
|
||||
@Test fun opensWhenThereIsNoSocket() {
|
||||
assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = false, sameUrl = false, socketActive = false))
|
||||
assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = false, sameUrl = true, socketActive = false))
|
||||
}
|
||||
|
||||
@Test fun reusesALiveSocketToTheSameUrl() {
|
||||
// A connected/self-healing socket to the same server -> reuse, never a duplicate.
|
||||
assertFalse(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = true))
|
||||
}
|
||||
|
||||
@Test fun opensWhenTheSocketWentInert() {
|
||||
// Socket exists but is inert (e.g. after `io server disconnect`, which Socket.IO does
|
||||
// not auto-reconnect) -> bring up one new connection.
|
||||
assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = false))
|
||||
}
|
||||
|
||||
@Test fun opensWhenTheUrlChanged() {
|
||||
// Genuine re-provision to a different server -> switch (do not reuse the old one).
|
||||
assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = false, socketActive = true))
|
||||
assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = false, socketActive = false))
|
||||
}
|
||||
|
||||
/**
|
||||
* THE #148 case: repeated connect() from repeated Activity binds / foreground re-binds must
|
||||
* never open a second socket while one is active. Simulate the 8-in-9s storm of binds.
|
||||
*/
|
||||
@Test fun idempotentAcrossManyRapidBinds() {
|
||||
repeat(8) { i ->
|
||||
assertFalse(
|
||||
"bind #$i must reuse the live socket, not open a duplicate",
|
||||
ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = true)
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue