Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions android/src/androidTest/java/com/comapeo/core/NodeJSIPCTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,45 @@ class NodeJSIPCTest {
Thread.sleep(500)
}

/**
* Pins the synchronous-close contract: after [NodeJSIPC.close] returns, the
* peer's blocking read has already observed EOF. A regression to the
* `disconnect()` teardown — which closes the socket behind
* `cancelAndJoin()` of a receive loop parked in a blocking `readFully` —
* would deadlock and leave the read blocked, leaking the FD across reloads.
*/
@Test
fun closeReleasesSocketSynchronously() {
val connected = CountDownLatch(1)
val readResult = java.util.concurrent.atomic.AtomicInteger(Int.MIN_VALUE)
val readReturned = CountDownLatch(1)

startMockServer { input, _ ->
connected.countDown()
try {
readResult.set(input.read())
} catch (e: IOException) {
// Some kernels surface peer-close as IOException; treat as EOF.
readResult.set(-1)
}
readReturned.countDown()
}

val ipc = NodeJSIPC(socketFile) { msg -> receivedMessages.add(msg) }
assertTrue("Should connect within 10s", connected.await(10, TimeUnit.SECONDS))
// Let the receive loop settle so we test steady-state close, not a
// connect-cancel race.
Thread.sleep(200)

ipc.close()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xhigh review — test asserts peer EOF but not client teardown (PLAUSIBLE)

closeReleasesSocketSynchronously checks the server read unblocked, but never asserts the client receive coroutine actually terminated. Because close() cancels the scope without joining, the receive coroutine can still be unwinding (woken into its IOExceptiondisconnect() path) when tearDown() closes serverSocket/boundSocket and deletes the socket file. A regression where close() returns before the client coroutine unwinds still passes here; the leaked coroutine racing teardown would surface only as a flaky failure in a later test. Consider asserting client-side teardown (a post-close settle + state assertion) as the sibling tests do.


assertTrue(
"Server-side read must unblock with EOF within 1s of close() returning",
readReturned.await(1, TimeUnit.SECONDS)
)
assertEquals(-1, readResult.get())
}

@Test
fun handlesServerDisconnect() {
val connected = CountDownLatch(1)
Expand Down
9 changes: 7 additions & 2 deletions android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,13 @@ class ComapeoCoreModule : Module() {
}

OnDestroy {
ipc.disconnect()
controlIpc.disconnect()
// OnCreate/OnDestroy bind to the Expo AppContext (JS-runtime
// lifetime), so they fire on every JS reload while this process
// stays alive. Close synchronously: the backend must see EOF on the
// old socket before the next OnCreate connects, otherwise the FD
// lingers and rpc-reflector listeners leak onto MapeoManager.
ipc.close()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xhigh review — lifecycle implications of disconnect()close() (2 findings)

  • Use-after-close (PLAUSIBLE). close() cancels the scope permanently, so the instance is dead afterward. If OnActivityEntersForeground fires ipc.connect() on this same instance after OnDestroy but before the next OnCreate swaps in a fresh NodeJSIPC, connect() passes its Disconnected guard but scope.launch on the cancelled scope never runs — state stays Disconnected, and subsequent postMessages trySend into a drained channel and vanish with no error surfaced to JS.
  • Suppressed STOPPED transition (PLAUSIBLE). The old disconnect() path delivered a Disconnecting/Disconnected (→ STOPPING/STOPPED) transition to the observer; close() cancels the collector, so on a final teardown not followed by OnCreate, JS consumers never observe STOPPED.

For the steady-state reload (OnDestroy→OnCreate) both are benign — listeners are recreated. They only bite the foreground-before-recreate and final-teardown orderings.

controlIpc.close()
}

OnActivityEntersForeground {
Expand Down
50 changes: 47 additions & 3 deletions android/src/main/java/com/comapeo/core/NodeJSIPC.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.delay
Expand Down Expand Up @@ -188,11 +189,15 @@ class NodeJSIPC(
}
}
}
// `shutdown` before `cancelAndJoin`: the receive loop is parked in a
// blocking `readFully` that `cancelAndJoin` cannot interrupt, so without
// first waking it the join blocks until the node backend sends a message
// or closes the socket — a deadlock when node is connected but idle.
// Same fix as close().
shutdownSocket()
connectJob?.cancelAndJoin()
connectJob = null
try { dataOutputStream?.close() } catch (_: Exception) {}
try { dataInputStream?.close() } catch (_: Exception) {}
try { socket.close() } catch (_: Exception) {}
closeStreamsAndSocket()
}
disconnectJob.invokeOnCompletion { cause ->
state.value = when (cause) {
Expand All @@ -217,6 +222,45 @@ class NodeJSIPC(
connect()
sendChannel.trySend(message)
}

/**
* Synchronous, terminal teardown for JS reload (process stays alive, so the
* fd must be closed here or it leaks until process death). `shutdown` must
* precede `close`: the receive loop is parked in a blocking `readFully` that
* holds the socket open until it returns, so `close()` alone never reaches
* the node backend — `shutdownInput` wakes the read, `shutdownOutput` sends FIN.
* Not reusable after close; construct a new instance.
*/
fun close() {
scope.cancel()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xhigh review — close() non-joining teardown (3 verified findings)

scope.cancel() here does not join the connect/send/receive coroutines, opening three teardown races:

  • Connect-race FD leak (CONFIRMED). If a reload lands while the socket is still connecting (Connecting), close() returns having torn down nothing (::socket.isInitialized is still false), then the surviving connect coroutine finishes and assigns the now-open LocalSocket to the field. That fd leaks until process death — the exact reload leak this change exists to prevent, in the connect window.
  • In-flight send vs. stream close (PLAUSIBLE). If the send coroutine is mid-out.write in sendMessageInternal when dataOutputStream?.close() runs, both touch the same stream unsynchronized. Caught + logged, so harmless-looking, but it's a use+close race the old disconnect() avoided by cancelAndJoin-ing first.
  • Dropped buffered sends (PLAUSIBLE). Cancelling the scope before sendChannel.close() kills the send job immediately, so a postMessage queued just before OnDestroy is discarded instead of drained. disconnect() closed the channel first precisely so the send loop could drain.

All three resolve if close() wakes the read (shutdown) and then joins the connect coroutine before closing streams — e.g. a bounded runBlocking { withTimeoutOrNull(..) { connectJob?.join() } }. That's a design change (the method is deliberately synchronous), so flagging rather than auto-applying. The connect-race leak is worth confirming on-device.

// Mark terminal before shutdownSocket() wakes the receive loop's blocking
// readFully: its IOException handler calls disconnect(), which then
// short-circuits on this state guard instead of relaunching teardown on
// the now-cancelled scope. Set after scope.cancel() so the (already
// cancelled) state collector still doesn't forward this transition,
// matching the prior terminal-close semantics.
state.value = State.Disconnected
sendChannel.close()
shutdownSocket()
closeStreamsAndSocket()
}

// `shutdown` (not `close`) wakes the receive loop parked in a blocking
// readFully and sends FIN; it must precede closeStreamsAndSocket().
private fun shutdownSocket() {
if (::socket.isInitialized) {
try { socket.shutdownInput() } catch (_: Exception) {}
try { socket.shutdownOutput() } catch (_: Exception) {}
}
}

private fun closeStreamsAndSocket() {
try { dataOutputStream?.close() } catch (_: Exception) {}
try { dataInputStream?.close() } catch (_: Exception) {}
if (::socket.isInitialized) {
try { socket.close() } catch (_: Exception) {}
}
}
}

/**
Expand Down
5 changes: 5 additions & 0 deletions ios/ComapeoCoreModule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public class ComapeoCoreModule: Module {
}

OnDestroy {
// Fires on every JS reload as of expo-modules-core SDK 53+ (weak
// MainValueConverter.appContext lets AppContext deinit; verified on
// 56). No iOS equivalent of the Android close() is needed: iOS
// disconnect() shutdown(2)s before joining the receive loop, so the
// backend sees EOF synchronously without the cancelAndJoin deadlock.
self.ipc?.disconnect()
self.ipc = nil
}
Expand Down