devadmin/ProLibertateGames
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Activity

Fix the hang that made a networked game impossible to leave

Browse Source 
Leaving a multiplayer game froze the app for good. Closing a link closed
the BufferedReader first, and BufferedReader.close() wants the very lock
readLine() holds for the whole of a blocking read — a read that only
closing the socket underneath it can ever end, which is the step the
close never reached. Both callers run on the thread drawing the screen
(AppRoot.pop and PlayScreen's onDispose), so the main thread waited on
itself and the app stopped responding. runCatching was no help: nothing
was thrown, it simply never came back.

StreamConnection.close() now closes only the raw streams, which is what
actually ends the read; the reader coroutine then closes its own wrapper
on its own thread, where it owns the lock. Leaving takes single-digit
milliseconds instead of never returning.

It bit chess because chess is the one game here worth leaving in the
middle of. A card hand is over in minutes; a game of chess between two
people who have had enough of it has no natural end, and there was
nothing to end it with, so:

  - Chess has an End game action, with a confirmation, so a table can be
    walked away from without hunting for a way out.

  - A link that drops on its own now reports itself on the same channel
    as everything else, so the device still at the table is told the
    other player has left rather than waiting on a move that is not
    coming. Bye was in the protocol and never sent; nothing needed to
    change on the wire for this.

  - Leaving a table no longer lands back in the lobby that started it. A
    lobby starts advertising the moment it is drawn, so "leave the game"
    used to put the player straight into another one.

Two crashes of the same shape on the same path, both in coroutines
launched in the screen's own scope where nothing catches anything:

  - The host's AI applied a move chosen before it took the lock, checking
    only that the seat had not changed. applyMove throws on a move that
    is no longer legal. Harmless while an AI answered instantly, which
    stopped being true when the chess search moved off-thread and started
    taking seconds. It is now checked against the position as it stands,
    and a stale choice is made again rather than forced.

  - A table the rules reject, or a state a client cannot decode, took the
    app down. Both are now reported.

Covered by MatchControllerNetworkTest, which plays a game over a real
socket pair and leaves it from either end. The leaving test runs on a
daemon thread and asserts it returns, so a regression fails the build
rather than hanging it — verified by reinstating the old close(), which
hangs the suite until it is killed.
This commit is contained in:
Claude
2026-08-06 14:44:01 +00:00
parent ee81fb7bc2
commit 0927e735a2
5 changed files with 468 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/org/prolibertate/games/net/MatchController.kt
+84 -7
View File
@@ -71,8 +71,22 @@ class MatchController<S : Any, M : Any>(
val finished: StateFlow<Boolean> = _finished.asStateFlow()
private val _notice = MutableStateFlow<String?>(null)
/** A passing remark about the last thing that happened, such as a refused move. */
val notice: StateFlow<String?> = _notice.asStateFlow()
private val _abandoned = MutableStateFlow<String?>(null)
/**
* Why this table can no longer be played, or null while it still can.
*
* Kept apart from [notice] because the two want opposite treatment: a
* refused move is worth a word in passing, whereas a game with nobody left
* to play it has to be brought to the player's attention along with the way
* out of it.
*/
val abandoned: StateFlow<String?> = _abandoned.asStateFlow()
private val _awaitingConfirmation = MutableStateFlow(false)
/** True while a scoreboard is up and the table is waiting to be released. */
@@ -91,6 +105,21 @@ class MatchController<S : Any, M : Any>(
private val seatPeers: Map<Int, String?> =
config.seats.associate { it.seat to it.peerId }
private fun nameOfPeer(peerId: String): String =
config.seats.firstOrNull { it.peerId == peerId }?.name ?: "The other player"
/**
* Records that this table cannot be played any further.
*
* Set on both channels on purpose: screens that show [notice] as a line of
* text carry on doing so, and one that wants to stop the player and offer
* the way out reads [abandoned] instead.
*/
private fun abandon(reason: String) {
_notice.value = reason
_abandoned.value = reason
}
// -----------------------------------------------------------------------
// Start-up
// -----------------------------------------------------------------------
@@ -105,8 +134,15 @@ class MatchController<S : Any, M : Any>(
}
}
scope.launch {
val initial = runCatching { rules.initialState(config) }.getOrElse { error ->
// A table the rules will not accept is a dead end, not a reason
// to take the app down with it: this runs in the screen's own
// scope, where nothing would catch it.
abandon("This table could not be set up: ${error.message}")
return@launch
}
lock.withLock {
authoritative = rules.initialState(config)
authoritative = initial
publish()
}
driveIdleSeats()
@@ -121,13 +157,30 @@ class MatchController<S : Any, M : Any>(
link.incoming.collect { message ->
when (message) {
is StateSync -> {
_state.value = rules.decodeState(message.stateJson)
_legalMoves.value = message.legalMoves.map { rules.decodeMove(it) }
_finished.value = _state.value?.let { rules.isFinished(it) } ?: false
// Anything off the wire is decoded defensively. This
// collector runs in the screen's own scope, so throwing
// here would take the app down rather than the message.
val decoded = runCatching {
val state = rules.decodeState(message.stateJson)
state to message.legalMoves.map { rules.decodeMove(it) }
}.getOrNull()
if (decoded == null) {
_notice.value = "The host sent something this version cannot read."
return@collect
}
_state.value = decoded.first
_legalMoves.value = decoded.second
_finished.value = rules.isFinished(decoded.first)
}
is Rejected -> _notice.value = message.reason
is Bye -> _notice.value = "The host left the game."
is Bye -> {
// The host has gone: the game cannot go on, and saying so
// is what gives the screen something to offer a way out of.
_legalMoves.value = emptyList()
abandon("The host left the game.")
}
else -> Unit
}
}
@@ -156,6 +209,13 @@ class MatchController<S : Any, M : Any>(
}
private suspend fun onHostMessage(connection: Connection, message: NetMessage) {
if (message is Bye) {
// One of the players has left. The host keeps the position — there is
// nothing wrong with it — but a table that cannot go on should say so
// rather than leave somebody waiting on a move that is not coming.
abandon("${nameOfPeer(connection.peerId)} left the game.")
return
}
if (message !is MoveIntent) return
val seat = seatPeers.entries.firstOrNull { it.value == connection.peerId }?.key
if (seat == null) {
@@ -244,13 +304,21 @@ class MatchController<S : Any, M : Any>(
val applied = lock.withLock {
val live = authoritative ?: return
// Nothing else can move while an AI seat is on the clock, but
// the state is re-read rather than assumed.
// the state is re-read rather than assumed. The move was chosen
// off this thread and a chess search takes seconds, so it is
// checked against the position as it stands now: applyMove
// throws on a move that is no longer legal, and there is nothing
// above this to catch it.
if (rules.currentSeat(live) != seat) return@withLock false
if (move !in rules.legalMoves(live, seat)) return@withLock false
authoritative = rules.applyMove(live, seat, move)
publish()
true
}
if (!applied) return
// The position moved on under the search. Go round again and pick a
// move for the position as it now stands; the loop returns on its own
// as soon as a person is on the clock.
if (!applied) continue
}
}
@@ -299,6 +367,15 @@ class MatchController<S : Any, M : Any>(
scope.launch { driveIdleSeats() }
}
/** Lets a player go on looking at a table they have been told is over. */
fun dismissAbandoned() {
_abandoned.value = null
}
/**
* Lets go of every link. Safe on the thread drawing the screen — see
* [StreamConnection.close], which this leans on to not block.
*/
fun close() {
peers.values.forEach { runCatching { it.close() } }
peers.clear()
app/src/main/java/org/prolibertate/games/net/Transport.kt
+40 -3
View File
@@ -2,6 +2,7 @@ package org.prolibertate.games.net
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.NonCancellable
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.SharedFlow
@@ -73,9 +74,16 @@ class StreamConnection(
init {
scope.launch(Dispatchers.IO) {
// Told apart from a link this side closed on purpose, because only
// one of the two is worth telling anybody about.
var droppedByPeer = false
try {
while (isActive && !closed.get()) {
val line = reader.readLine() ?: break
val line = reader.readLine()
if (line == null) {
droppedByPeer = true
break
}
if (line.isBlank()) continue
val message = runCatching {
protocolJson.decodeFromString<NetMessage>(line)
@@ -85,8 +93,23 @@ class StreamConnection(
} catch (_: Exception) {
// A dropped link is normal: someone walked out of range or
// closed the app. Fall through to close().
droppedByPeer = true
} finally {
// Safe here and nowhere else: this thread owns the reader's
// lock, so it is the only one that can close it without
// waiting. See close().
runCatching { reader.close() }
val weClosedIt = closed.get()
close()
// A link that died on its own reaches the screens on the same
// channel as everything else, so leaving a game is something the
// other end finds out about the same way it finds out about a
// move. Withdrawing deliberately needs no announcement, and the
// emit is not cancellable — the whole point of it is to arrive
// while everything around it is being torn down.
if (droppedByPeer && !weClosedIt) {
withContext(NonCancellable) { runCatching { _incoming.emit(Bye) } }
}
}
}
}
@@ -104,10 +127,24 @@ class StreamConnection(
}
}
/**
* Safe to call from any thread, including the one drawing the screen.
*
* Only the raw streams are closed here, and that is deliberate. A reader
* parked in [BufferedReader.readLine] holds the reader's own lock for the
* whole of the blocking read, and [BufferedReader.close] wants that same
* lock — so closing the wrapper from another thread waits for a read that
* only this close can end. Since the read is waiting on the socket, and the
* caller is usually the main thread leaving a game, that wait is the app
* hanging for good.
*
* Closing the underlying stream is what actually ends the read. The reader
* coroutine then unblocks and closes its own wrapper on its own thread. The
* writer needs no closing: every [send] flushes, so nothing is ever left
* buffered in it.
*/
override fun close() {
if (!closed.compareAndSet(false, true)) return
runCatching { reader.close() }
runCatching { writer.close() }
runCatching { input.close() }
runCatching { output.close() }
onClosed(this)
app/src/main/java/org/prolibertate/games/ui/AppRoot.kt
+13 -2
View File
@@ -86,12 +86,23 @@ fun AppRoot(env: AppEnv) {
val pop: () -> Unit = {
if (stack.size > 1) {
val leaving = stack.last()
// Leaving a lobby or a table tears the networking down with it.
when (stack.last()) {
when (leaving) {
is Route.Lobby, is Route.Play -> env.lobby.stop()
else -> Unit
}
stack = stack.dropLast(1)
stack = stack.dropLast(1).let { remaining ->
// Leaving a table must not land back in the lobby that started
// it. A lobby starts advertising the moment it is drawn, so
// ending a game would have put the player straight into another
// one — which is no way to leave a game.
if (leaving is Route.Play) {
remaining.dropLastWhile { it is Route.Lobby }
} else {
remaining
}
}
}
}
app/src/main/java/org/prolibertate/games/ui/game/ChessScreen.kt
+70 -10
View File
@@ -72,16 +72,32 @@ fun ChessScreen(
) {
val state by controller.state.collectAsState()
val legal by controller.legalMoves.collectAsState()
val abandoned by controller.abandoned.collectAsState()
var selected by remember { mutableStateOf<Int?>(null) }
// A pawn reaching the last rank is one tap that stands for four moves, so
// the destination is held while the piece is chosen.
var promotionTo by remember { mutableStateOf<Int?>(null) }
// A game with nobody left to play it, or one being walked away from, both end
// through a dialog rather than by silently dropping off the screen.
var confirmingEnd by remember { mutableStateOf(false) }
val endGameAction: @Composable () -> Unit = {
TextButton(onClick = { confirmingEnd = true }) { Text("End game") }
}
val current = state
if (current == null) {
ScreenScaffold(title = "Chess", onBack = onExit) { modifier ->
ScreenScaffold(title = "Chess", onBack = onExit, actions = endGameAction) { modifier ->
Box(modifier, contentAlignment = Alignment.Center) { Text("Setting up…") }
// A table that never arrives — a host that went away mid-handshake —
// still has to be leavable.
abandoned?.let {
LeftTheTableDialog(it, onExit = onExit, onStay = controller::dismissAbandoned)
}
if (confirmingEnd) {
ConfirmEndDialog(onEnd = onExit, onKeepPlaying = { confirmingEnd = false })
}
}
return
}
@@ -104,7 +120,7 @@ fun ChessScreen(
}
}
ScreenScaffold(title = "Chess", onBack = onExit) { modifier ->
ScreenScaffold(title = "Chess", onBack = onExit, actions = endGameAction) { modifier ->
BoxWithConstraints(modifier = modifier) {
val wide = maxWidth > maxHeight
val boardSide = if (wide) {
@@ -152,10 +168,21 @@ fun ChessScreen(
}
}
promotionTo?.let { destination ->
PromotionDialog(
// One dialog at a time, most pressing first. A game nobody can finish
// comes before the result of one that finished, and before a
// half-made promotion: there is no point choosing a piece for a move
// that has nowhere left to go.
val over = abandoned
when {
over != null ->
LeftTheTableDialog(over, onExit = onExit, onStay = controller::dismissAbandoned)
confirmingEnd ->
ConfirmEndDialog(onEnd = onExit, onKeepPlaying = { confirmingEnd = false })
promotionTo != null -> PromotionDialog(
white = current.whiteToMove,
choices = fromSelected.filter { it.to == destination },
choices = fromSelected.filter { it.to == promotionTo },
onPick = {
controller.submit(it)
promotionTo = null
@@ -163,13 +190,46 @@ fun ChessScreen(
},
onDismiss = { promotionTo = null },
)
current.phase == ChessPhase.GAME_OVER -> ResultDialog(current, onExit)
}
}
}
}
if (current.phase == ChessPhase.GAME_OVER) {
ResultDialog(current, onExit)
}
}
/**
* Leaving a game on purpose.
*
* Chess is the one game here that a player wants out of before the position is
* settled: a card hand is over in minutes, whereas a game of chess between two
* people who are no longer enjoying it has no natural end at all.
*/
@Composable
private fun ConfirmEndDialog(onEnd: () -> Unit, onKeepPlaying: () -> Unit) {
AlertDialog(
onDismissRequest = onKeepPlaying,
title = { Text("End this game?") },
text = {
Text(
"This game ends here and the position is not saved. Anyone else " +
"at the table is told you have left."
)
},
confirmButton = { TextButton(onClick = onEnd) { Text("End the game") } },
dismissButton = { TextButton(onClick = onKeepPlaying) { Text("Keep playing") } },
)
}
/** The other end has gone, so no further move is ever going to arrive. */
@Composable
private fun LeftTheTableDialog(notice: String, onExit: () -> Unit, onStay: () -> Unit) {
AlertDialog(
onDismissRequest = onStay,
title = { Text("The game has ended") },
text = { Text("$notice There are no more moves to play.") },
confirmButton = { TextButton(onClick = onExit) { Text("Leave the game") } },
dismissButton = { TextButton(onClick = onStay) { Text("Look at the board") } },
)
}
@Composable
@@ -372,7 +432,7 @@ private fun ResultDialog(state: ChessState, onExit: () -> Unit) {
)
}
},
confirmButton = { TextButton(onClick = onExit) { Text("Back to the menu") } },
confirmButton = { TextButton(onClick = onExit) { Text("Leave the game") } },
dismissButton = { TextButton(onClick = { dismissed = true }) { Text("Look at the board") } },
)
}
app/src/test/java/org/prolibertate/games/net/MatchControllerNetworkTest.kt
+262
View File
@@ -0,0 +1,262 @@
package org.prolibertate.games.net
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.cancel
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.prolibertate.games.game.GameCatalog
import org.prolibertate.games.game.chess.ChessAi
import org.prolibertate.games.game.chess.ChessMove
import org.prolibertate.games.game.chess.ChessOptions
import org.prolibertate.games.game.chess.ChessOutcome
import org.prolibertate.games.game.chess.ChessPhase
import org.prolibertate.games.game.chess.ChessRules
import org.prolibertate.games.game.chess.ChessState
import org.prolibertate.games.game.chess.squareFromName
import org.prolibertate.games.game.engine.PlayerKind
import org.prolibertate.games.game.engine.PlayerSlot
import org.prolibertate.games.game.engine.TableConfig
import java.net.InetSocketAddress
import java.net.ServerSocket
import java.net.Socket
/**
* Two devices playing one networked game, wired up the way PlayScreen wires it,
* over a real socket pair on loopback.
*
* The engines are covered exhaustively elsewhere; what is worth testing here is
* the part no rules test can reach — a game with a live connection in it being
* played, finished, and above all *left*, which is where a networked table has
* to let go of a socket while the screen that owns it is being torn down.
*/
class MatchControllerNetworkTest {
private val tables = mutableListOf<Table>()
@After
fun tearDown() {
tables.forEach { it.dispose() }
}
private fun table(): Table = Table().also { tables += it }
// -----------------------------------------------------------------------
@Test
fun `a networked game plays to a result on both devices`() = runBlocking {
val table = table()
table.settle()
assertEquals(ChessPhase.PLAYING, table.client.state.value?.phase)
// Black is not on the clock, so has nothing to play.
assertEquals(0, table.client.legalMoves.value.size)
table.white("f2", "f3")
table.black("e7", "e5")
table.white("g2", "g4")
table.black("d8", "h4")
assertEquals(ChessOutcome.BLACK_WINS, table.host.state.value?.outcome)
assertEquals(ChessOutcome.BLACK_WINS, table.client.state.value?.outcome)
assertTrue(table.host.finished.value)
assertTrue(table.client.finished.value)
}
/**
* The regression this file exists for.
*
* Leaving a game runs on the thread drawing the screen — Android's main
* thread — by way of AppRoot.pop() and PlayScreen's onDispose. It closed the
* buffered reader first, which wants the same lock readLine() holds for the
* whole of a blocking read, so the main thread waited on a read that only its
* own close could ever end. The app hung for good, and a chess game is the
* one long enough that a player wants out of it mid-game.
*/
@Test
fun `leaving a game in progress does not block the thread that leaves`() = runBlocking {
for (side in listOf(Side.HOST, Side.GUEST)) {
val table = table()
table.settle()
table.white("e2", "e4")
// Left on its own thread, and a daemon one: should this ever hang
// again, the test has to report it rather than take the build down
// with it — a thread stuck on a lock cannot be interrupted.
val left = Thread { table.leave(side) }.apply { isDaemon = true }
left.start()
left.join(BLOCKED_MILLIS)
assertTrue(
"Leaving as $side did not return: the thread that ends a game is stuck in close()",
!left.isAlive,
)
}
}
@Test
fun `a player leaving is reported to the device still at the table`() = runBlocking {
val guestLeft = table()
guestLeft.settle()
guestLeft.leave(Side.GUEST)
guestLeft.settle()
assertEquals("Guest left the game.", guestLeft.host.abandoned.value)
val hostLeft = table()
hostLeft.settle()
hostLeft.leave(Side.HOST)
hostLeft.settle()
assertEquals("The host left the game.", hostLeft.client.abandoned.value)
// Nothing more can be played on a table with no host.
assertEquals(0, hostLeft.client.legalMoves.value.size)
}
@Test
fun `closing a link on purpose is not reported as the other player leaving`() = runBlocking {
val table = table()
table.settle()
assertNull(table.host.abandoned.value)
// The host closes its own end, as leaving does. Its own screen has no
// business being told that somebody else left.
table.hostLink.close()
table.settle()
assertNull(table.host.abandoned.value)
// The guest, on the other hand, has lost the game it was playing.
assertNotNull(table.client.abandoned.value)
}
// -----------------------------------------------------------------------
private enum class Side { HOST, GUEST }
/** One two-seat chess table with a real socket between the two devices. */
private class Table {
private val server = ServerSocket(0)
private val clientSocket = Socket()
private val hostSocket: Socket
private val hostScope = CoroutineScope(Job() + Dispatchers.Default)
private val clientScope = CoroutineScope(Job() + Dispatchers.Default)
val hostLink: StreamConnection
private val clientLink: StreamConnection
val host: MatchController<ChessState, ChessMove>
val client: MatchController<ChessState, ChessMove>
init {
clientSocket.connect(InetSocketAddress("127.0.0.1", server.localPort), CONNECT_MILLIS)
hostSocket = server.accept()
val guestPeerId = hostSocket.inetAddress?.hostAddress ?: "guest"
hostLink = StreamConnection(
peerId = guestPeerId,
kind = TransportKind.LAN,
input = hostSocket.getInputStream(),
output = hostSocket.getOutputStream(),
scope = hostScope,
onClosed = { runCatching { hostSocket.close() } },
)
clientLink = StreamConnection(
peerId = "lan:host",
kind = TransportKind.LAN,
input = clientSocket.getInputStream(),
output = clientSocket.getOutputStream(),
scope = clientScope,
onClosed = { runCatching { clientSocket.close() } },
)
val config = TableConfig(
gameId = GameCatalog.CHESS,
seats = listOf(
PlayerSlot(0, "Host", PlayerKind.HUMAN_LOCAL, team = 0),
PlayerSlot(1, "Guest", PlayerKind.HUMAN_REMOTE, team = 1, peerId = guestPeerId),
),
optionsJson = Json { encodeDefaults = true }.encodeToString(ChessOptions()),
seed = 1L,
)
fun controller(role: MatchController.Role, seat: Int, scope: CoroutineScope) =
MatchController<ChessState, ChessMove>(
rules = ChessRules,
ai = ChessAi(),
config = config,
scope = scope,
role = role,
localSeats = setOf(seat),
primarySeat = seat,
aiThinkingMillis = { 0L },
)
host = controller(MatchController.Role.HOST, 0, hostScope)
client = controller(MatchController.Role.CLIENT, 1, clientScope)
client.startAsClient(clientLink)
host.startAsHost(listOf(hostLink))
}
/** Lets whatever is in flight cross the socket and be applied. */
suspend fun settle() = delay(SETTLE_MILLIS)
suspend fun white(from: String, to: String) {
host.submit(move(from, to))
settle()
}
suspend fun black(from: String, to: String) {
client.submit(move(from, to))
settle()
}
/** Exactly what AppRoot.pop() and PlayScreen's onDispose do, in that order. */
fun leave(side: Side) {
if (side == Side.HOST) {
runCatching { hostLink.close() }
host.close()
hostScope.cancel()
} else {
runCatching { clientLink.close() }
client.close()
clientScope.cancel()
}
}
/**
* Sockets first, on purpose: that is what frees any reader still parked
* on one, so clearing up after a failed test cannot itself hang.
*/
fun dispose() {
runCatching { hostSocket.close() }
runCatching { clientSocket.close() }
runCatching { server.close() }
runCatching { hostLink.close() }
runCatching { clientLink.close() }
runCatching { host.close() }
runCatching { client.close() }
hostScope.cancel()
clientScope.cancel()
}
private fun move(from: String, to: String) =
ChessMove(squareFromName(from), squareFromName(to))
}
private companion object {
const val CONNECT_MILLIS = 2_000
const val SETTLE_MILLIS = 300L
/**
* Generous on purpose: leaving takes single-digit milliseconds, and the
* failure this guards against is not a slow close but one that never
* returns at all.
*/
const val BLOCKED_MILLIS = 5_000L
}
}