diff --git a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt index a0ba24f34a..425bf343f9 100644 --- a/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt +++ b/lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt @@ -146,27 +146,31 @@ internal class JavaDebugAdapter : _listenerState?.invalidate() listenerThread?.interrupt() - - _listenerState = + + // Held locally as well as in the field: close() may null the field from another thread, and + // re-reading it with !! below would then throw (ADFA-5398). + val state = ListenerState( client = client, connector = connector, args = args, ) + _listenerState = state - val failure = withContext(Dispatchers.IO) { - try { - logger.debug("startListening") - listenerState.startListening() - null - } catch (e: Throwable) { - if (e is CancellationException) { - throw e + val failure = + withContext(Dispatchers.IO) { + try { + logger.debug("startListening") + listenerState.startListening() + null + } catch (e: Throwable) { + if (e is CancellationException) { + throw e + } + logger.error("Failed to listen for incoming JDWP connections", e) + return@withContext DebugClientConnectionResult.Failure(cause = e) } - logger.error("Failed to listen for incoming JDWP connections", e) - return@withContext DebugClientConnectionResult.Failure(cause = e) } - } if (failure != null) { return failure @@ -174,7 +178,7 @@ internal class JavaDebugAdapter : listenerThread = JDWPListenerThread( - _listenerState!!, + state, this::onConnectedToVm, ).also { thread -> thread.start() } return DebugClientConnectionResult.Success @@ -237,7 +241,20 @@ internal class JavaDebugAdapter : threadState.initThreads() this.vms.add(vmConnection) - this._listenerState!!.client.onAttach(client) + + val listener = this._listenerState + if (listener == null) { + // close() ran while this connection was being established. Returning here without + // undoing the two lines above would leave a live VM connection, and its event handler, + // attached to an adapter that is gone. + logger.warn("Connected to a VM after the debug adapter was closed; dropping it") + vms.remove(vmConnection) + runCatching { vmConnection.close() } + .onFailure { err -> logger.error("Failed to close a VM connected after close()", err) } + return + } + + listener.client.onAttach(client) } override suspend fun connectedRemoteClients(): Set = vms.map(VmConnection::client).toSet() @@ -354,7 +371,7 @@ internal class JavaDebugAdapter : val spec = when (breakpoint) { - is PositionalBreakpoint -> + is PositionalBreakpoint -> { specList.createBreakpoint( source = breakpoint.source, // +1 because we receive 0-indexed line numbers from the IDE @@ -363,8 +380,9 @@ internal class JavaDebugAdapter : qualifiedName = qualifiedName, suspendPolicy = breakpoint.suspendPolicy.asJdiInt(), ) + } - is MethodBreakpoint -> + is MethodBreakpoint -> { specList.createBreakpoint( source = breakpoint.source, methodId = breakpoint.methodId, @@ -372,8 +390,11 @@ internal class JavaDebugAdapter : qualifiedName = qualifiedName, suspendPolicy = breakpoint.suspendPolicy.asJdiInt(), ) + } - else -> throw IllegalArgumentException("Unsupported breakpoint type: $breakpoint") + else -> { + throw IllegalArgumentException("Unsupported breakpoint type: $breakpoint") + } } val result = @@ -385,19 +406,23 @@ internal class JavaDebugAdapter : val resolveSuccess = result.getOrDefault(false) when { - resolveSuccess && spec.isResolved -> + resolveSuccess && spec.isResolved -> { BreakpointResult.Success( breakpoint, false, ) + } - resolveSuccess && !spec.isResolved -> + resolveSuccess && !spec.isResolved -> { BreakpointResult.Success( breakpoint, true, ) + } - else -> BreakpointResult.Failure(breakpoint, failure) + else -> { + BreakpointResult.Failure(breakpoint, failure) + } } }, ) @@ -584,11 +609,22 @@ internal class JavaDebugAdapter : override fun close() { logger.debug("close") + // Separate catches: the listener thread holds its own ListenerState reference, so if + // invalidate() throws and skips the interrupt, clearing the fields below releases nothing. + runCatching { _listenerState?.invalidate() } + .onFailure { err -> logger.error("Unable to invalidate the VM connection listener", err) } + try { - _listenerState?.invalidate() listenerThread?.interrupt() } catch (err: Throwable) { logger.error("Unable to stop VM connection listener", err) + } finally { + // ListenerState holds the IDebugClient, which holds the DebuggerViewModel and its whole + // thread/frame/variable state. This adapter lives on JavaLanguageServer in the global + // registry, so without dropping the reference here that graph stays reachable from a + // native GC root until the next connectDebugClient (ADFA-5398). + _listenerState = null + listenerThread = null } adapterScope.launch(Dispatchers.IO) { @@ -631,8 +667,10 @@ internal class JDWPListenerThread( override fun run() { logger.debug("run::start") if (!listenerState.isListening && !listenerState.isInvalidated) { - logger.warn("Listener should've been listening at this point, but it's not. " + - "Trying to start listening...") + logger.warn( + "Listener should've been listening at this point, but it's not. " + + "Trying to start listening...", + ) listenerState.startListening() }