ADFA-3191: warn users when the IDE is not allowed to access the local network#1088
Open
itsaky-adfa wants to merge 11 commits intostagefrom
Open
ADFA-3191: warn users when the IDE is not allowed to access the local network#1088itsaky-adfa wants to merge 11 commits intostagefrom
itsaky-adfa wants to merge 11 commits intostagefrom
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
📝 WalkthroughWalkthroughRefactors the language server registry from Java singleton to a Kotlin companion-backed registry, converts debug connection APIs to suspending functions returning per-server results, adds debug-adapter readiness checks and aggregated error handling during startup, and introduces related string resources and minor call-site adaptations. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as ProjectHandlerActivity
participant Registry as ILanguageServerRegistry.default
participant JavaLS as JavaLanguageServer
participant JavaDA as JavaDebugAdapter
participant UI as MaterialDialog
Activity->>Registry: initLspClient() (suspend)
Registry->>JavaLS: register/connect servers
Activity->>Registry: connectDebugClient(IDebugClient) (suspend)
Registry->>JavaLS: JavaLS.connectDebugClient(client) (suspend)
JavaLS->>JavaDA: debugAdapter.connectDebugClient(client) (suspend)
JavaDA-->>JavaLS: DebugClientConnectionResult (Success|Failure)
JavaLS-->>Registry: per-server DebugClientConnectionResult
Registry-->>Activity: Map<serverId, DebugClientConnectionResult>
Activity->>Activity: aggregate failures
alt Any Failures
Activity->>UI: show aggregated failure dialog
else All Success
Activity->>Activity: continue startup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/src/main/java/com/itsaky/androidide/editor/language/treesitter/KotlinLanguage.kt (1)
42-42:⚠️ Potential issue | 🔴 CriticalAdd missing
Factoryimport.Line 42 references
Factoryunqualified, but this file does not importTreeSitterLanguage.Factory. All peer language implementations (JavaLanguage, JsonLanguage, etc.) import it explicitly. Without the import, this is an unresolved reference and blocks compilation.🔧 Proposed fix
import com.itsaky.androidide.editor.language.treesitter.TreeSitterLanguage.Factory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/main/java/com/itsaky/androidide/editor/language/treesitter/KotlinLanguage.kt` at line 42, KotlinLanguage references an unqualified Factory causing an unresolved symbol; add the missing import for TreeSitterLanguage.Factory to the top of the file (same style used in JavaLanguage/JsonLanguage) so the line "val FACTORY = Factory { KotlinLanguage(it) }" compiles; locate the KotlinLanguage class and add the import for TreeSitterLanguage.Factory to resolve Factory.lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt (1)
147-153:⚠️ Potential issue | 🟠 MajorDon’t swap listener state until the previous listener is handled.
This replaces
_listenerStatebefore the existing listener has been reused or shut down.app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt, Lines 970-979, callsconnectDebugClient(...)directly, andapp/src/main/java/com/itsaky/androidide/actions/build/DebugAction.kt, Lines 85-91, gates the flow onisReady. A second call here can leave the oldJDWPListenerThreadbound to the socket while the adapter now reports “not ready”. Please guard re-entry here or stop the old listener before swapping state.Also applies to: 165-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt` around lines 147 - 153, The code currently replaces _listenerState with a new ListenerState(client, connector, args) immediately, which can leave an old JDWPListenerThread bound; modify connectDebugClient (and the same pattern around the other instance at the 165-175 area) to guard re-entry: if an existing _listenerState exists, either stop/shutdown its JDWPListenerThread (call its stop/shutdown method) and wait for it to fully exit before assigning the new ListenerState, or return early if the adapter is not ready, using isReady to gate the swap; ensure you reference and operate on the existing _listenerState and its JDWPListenerThread rather than blindly overwriting it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/actions/build/DebugAction.kt`:
- Around line 85-90: preExec calls showDebuggerNotReadyMessage(...) directly on
a potential background thread; wrap that call in
withContext(Dispatchers.Main.immediate) so the AlertDialog is created/shown on
the main thread. Locate the javaLsp check around
ILanguageServerRegistry.default.getServer(JavaLanguageServer.SERVER_ID) in
preExec and replace the direct showDebuggerNotReadyMessage(activity) invocation
with a withContext(Dispatchers.Main.immediate) {
showDebuggerNotReadyMessage(activity) } block to match the other dialog paths.
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt`:
- Around line 973-979: The current catch in the debug-client handshake is too
broad (catching Throwable) and will swallow coroutine cancellations; change the
handler in the connectDebugClient(...) call so it only catches Exception, but
explicitly rethrow CancellationException if encountered (i.e., if (e is
CancellationException) throw e), then perform Sentry.captureException and
logger.error and return listOf(DebugClientConnectionResult.Failure(cause = e))
for other exceptions; update the try/catch around
connectDebugClient(debuggerViewModel.debugClient).values accordingly to use
Exception and rethrow CancellationException.
In
`@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt`:
- Around line 125-130: The current write-lock block in
DefaultLanguageServerRegistry wrongly restores the old instance after inserting
the new one: the mRegister.put(server.serverId, server) result (old) is
immediately put back, preventing updates; fix by deciding intended behavior and
implementing it—either detect duplicate server.serverId and throw/return early
(e.g., check mRegister.containsKey(server.serverId) and fail fast) or keep the
new instance and remove/retire the old (remove the mRegister[old.serverId] = old
line so the new server remains and perform any retirement/cleanup on old).
Ensure changes touch the lock.writeLock() block that uses
mRegister.put(server.serverId, server) and the local variable old.
- Around line 136-140: The unregister(serverId: String) method currently removes
the server from mRegister and throws if absent but never calls shutdown() on the
removed instance; change it to acquire the write lock, remove the entry from
mRegister, if the removed value is null simply return (do not throw), release
the lock, and then call shutdown() on the removed server instance outside the
lock to avoid holding the lock during shutdown; ensure the logic references
unregister, mRegister, and shutdown() so the removed server is shut down after
removal.
- Around line 63-72: In DefaultLanguageServerRegistry.kt, in the block where you
call server.connectDebugClient(client) (which currently catches Throwable and
maps everything to DebugClientConnectionResult.Failure), change the error
handling to re-throw CancellationException immediately so coroutine cancellation
is preserved, and replace the broad catch(Throwable) with catches for the
concrete exceptions that connectDebugClient() can actually throw (log via
sLogger.error and convert those specific exceptions to
DebugClientConnectionResult.Failure); ensure you still log the server.serverId
and the exception when returning Failure but never swallow or convert
CancellationException.
- Around line 55-77: The method connectDebugClient currently holds
lock.readLock() across suspend calls (server.connectDebugClient), blocking
writers; instead, while holding lock.readLock() copy/snapshot the registry
entries (mRegister.values) into a local list and then release the read lock
before iterating and calling the suspend function
server.connectDebugClient(client); build the result map from that snapshot and
preserve the existing error handling (sLogger.error and mapping to
DebugClientConnectionResult.Failure) while ensuring lock.readLock().unlock()
happens immediately after taking the snapshot.
- Around line 111-117: In DefaultLanguageServerRegistry::onProjectInitialized,
acquire the registry read lock before reading mRegister to avoid races with
register()/unregister(); wrap the iteration over mRegister.values and the calls
to server.setupWithProject(project) inside lock.readLock() (or use the
readLock().lock()/unlock() pattern or a try/finally) so access is consistent
with other methods that guard mRegister.
In `@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServer.kt`:
- Line 83: The default implementation of connectDebugClient currently always
returns DebugClientConnectionResult.Success causing false positives; change it
so the method delegates to debugAdapter?.connectDebugClient(client) and return
that result when debugAdapter is non-null, otherwise return an explicit failure
(e.g., DebugClientConnectionResult.Failure or a suitable error variant) so
callers see unsupported/uninitialized debug adapter; update the suspend fun
connectDebugClient(client: IDebugClient): DebugClientConnectionResult to perform
this delegation and null-check.
In
`@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.kt`:
- Around line 71-81: The singleton getter for ILanguageServerRegistry (sRegistry
/ default) is not thread-safe; replace the nullable backing var and manual
check-then-set with a synchronized lazy initialization: remove sRegistry, make
default a val initialized via Kotlin's lazy with thread-safety (e.g., val
default: ILanguageServerRegistry by lazy(LazyThreadSafetyMode.SYNCHRONIZED) {
DefaultLanguageServerRegistry() }) so DefaultLanguageServerRegistry is created
once safely across threads.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`:
- Around line 627-630: The recovery path in run() that calls
listenerState.startListening() when listenerState.isListening is false can
reopen the socket after a prior close(); before attempting to restart, check the
adapter's shutdown/interrupted state (e.g., a shutdownRequested flag or
Thread.currentThread().isInterrupted()) and skip the self-heal if shutdown was
requested, or alternatively remove the self-heal logic entirely; update the
block around listenerState.isListening and startListening() so it first
returns/exits the loop when a shutdown/interruption is observed, otherwise
proceed to call startListening().
- Around line 154-163: The current catch in the withContext block catches
Throwable and therefore swallows coroutine cancellation; change the error
handling around listenerState.startListening() so CancellationException is not
absorbed: add "import kotlinx.coroutines.CancellationException", narrow the
catch from Throwable to Exception (or explicitly catch Exception), and if you
must catch Throwable ensure you rethrow CancellationException (i.e., if (e is
CancellationException) throw e) before logging/returning the Failure; update the
block around listenerState.startListening() and the catch that currently logs
"Failed to listen for incoming JDWP connections" to follow this pattern.
---
Outside diff comments:
In
`@editor/src/main/java/com/itsaky/androidide/editor/language/treesitter/KotlinLanguage.kt`:
- Line 42: KotlinLanguage references an unqualified Factory causing an
unresolved symbol; add the missing import for TreeSitterLanguage.Factory to the
top of the file (same style used in JavaLanguage/JsonLanguage) so the line "val
FACTORY = Factory { KotlinLanguage(it) }" compiles; locate the KotlinLanguage
class and add the import for TreeSitterLanguage.Factory to resolve Factory.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`:
- Around line 147-153: The code currently replaces _listenerState with a new
ListenerState(client, connector, args) immediately, which can leave an old
JDWPListenerThread bound; modify connectDebugClient (and the same pattern around
the other instance at the 165-175 area) to guard re-entry: if an existing
_listenerState exists, either stop/shutdown its JDWPListenerThread (call its
stop/shutdown method) and wait for it to fully exit before assigning the new
ListenerState, or return early if the adapter is not ready, using isReady to
gate the swap; ensure you reference and operate on the existing _listenerState
and its JDWPListenerThread rather than blindly overwriting it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc0db31a-cc9b-42c1-b8b4-5787b722d825
📒 Files selected for processing (20)
app/src/main/java/com/itsaky/androidide/actions/build/DebugAction.ktapp/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/handlers/LspHandler.ktapp/src/main/java/com/itsaky/androidide/ui/CodeEditorView.kteditor/src/main/java/com/itsaky/androidide/editor/language/treesitter/JavaLanguage.kteditor/src/main/java/com/itsaky/androidide/editor/language/treesitter/KotlinLanguage.kteditor/src/main/java/com/itsaky/androidide/editor/language/treesitter/XMLLanguage.kteditor/src/main/java/com/itsaky/androidide/editor/ui/EditorActionsMenu.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.javalsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServer.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.javalsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/debug/DebugClientConnectionResult.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/debug/IDebugAdapter.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/BaseJavaCodeAction.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.ktlsp/xml/src/main/java/com/itsaky/androidide/lsp/xml/providers/AdvancedEditProvider.ktresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (2)
- lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.java
- lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.java
app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt
Outdated
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt
Outdated
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServer.kt
Outdated
Show resolved
Hide resolved
lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.kt
Outdated
Show resolved
Hide resolved
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt
Show resolved
Hide resolved
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/ListenerState.kt (1)
48-53:⚠️ Potential issue | 🟠 MajorFlip
invalidatedbeforestopListening().
invalidate()currently closes the socket first and only then marks the state invalid. That leaves a race whereJDWPListenerThread.run()can observe!isListening && !isInvalidatedand reopen the listener during shutdown or replacement.Suggested fix
fun invalidate() { - if (isListening) { - stopListening() - } - - invalidated.set(true) + if (!invalidated.compareAndSet(false, true)) { + return + } + if (isListening) { + stopListening() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/ListenerState.kt` around lines 48 - 53, The invalidate() method flips invalidated after stopListening(), which allows JDWPListenerThread.run() to see !isListening && !isInvalidated and restart the listener; change invalidate() to set invalidated.set(true) before calling stopListening() so the run loop sees the invalidated flag immediately and will not reopen the listener during shutdown/replacement (refer to invalidate(), invalidated, stopListening() and JDWPListenerThread.run()).lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt (2)
136-145:⚠️ Potential issue | 🟠 MajorKeep
unregister()a no-op when the server is already absent.The API contract says “If any server is registered...”, so throwing on a miss makes teardown non-idempotent and can crash defensive cleanup paths. Return when
remove()isnull, then shut down only the removed instance.Suggested fix
override fun unregister(serverId: String) { - val registered = lock.writeLock().withLock { - mRegister.remove(serverId) - } - - checkNotNull(registered) { "No server found for the given server ID" } + val registered = lock.writeLock().withLock { + mRegister.remove(serverId) + } ?: return try { registered.shutdown()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt` around lines 136 - 145, The unregister method currently throws when mRegister.remove(serverId) returns null; change it to be a no-op on missing servers by returning early if the removed value is null. In the lock.writeLock().withLock block call mRegister.remove(serverId) into registered and, immediately after the lock, if registered is null simply return; only call registered.shutdown() inside the try/catch for a non-null registered instance (keeping the existing try/catch around registered.shutdown()) so teardown is idempotent.
62-78:⚠️ Potential issue | 🟠 MajorDon't convert fatal errors into per-server
Failureresults.This loop already preserves
CancellationException, butcatch (e: Throwable)still absorbs fatal JVM errors such asOutOfMemoryErrororLinkageErrorand keeps the coroutine running. CatchException(or the concrete connection exceptions) here instead of normalizing everyThrowable.Based on learnings: In Kotlin files across the AndroidIDE project, prefer narrow exception handling that catches only the specific exception type reported in crashes instead of a broad catch-all.Suggested fix
return buildMap { for (server in servers) { try { this[server.serverId] = server.connectDebugClient(client) - } catch (e: Throwable) { - if (e is CancellationException) { - throw e - } - + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { sLogger.error( "Unable to connect LSP server '{}' to debug client", server.serverId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt` around lines 62 - 78, The loop in DefaultLanguageServerRegistry.kt currently catches Throwable and converts fatal JVM errors into per-server Failure; change the catch to catch Exception instead of Throwable (but keep the existing check that rethrows CancellationException) so we only handle normal exceptions from server.connectDebugClient and still rethrow CancellationException; update the catch block that logs via sLogger.error for server.serverId and assigns DebugClientConnectionResult.Failure(cause = e) to use the Exception type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt`:
- Around line 83-84: Make the EventBus subscription lifecycle idempotent and
thread-safe by (1) guarding EventBus.getDefault().unregister(this) inside
destroy() with EventBus.getDefault().isRegistered(this) so unregister is only
called when subscribed, and (2) moving the isRegistered() check currently in
register() into the same write-lock critical section where the registry map is
updated so registration and the map update occur atomically; update the
register() method to acquire the write lock, check isRegistered(), then call
register(this) and update the map only if not already registered.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`:
- Around line 148-178: Currently you reassign _listenerState before tearing down
the previous JDWPListenerThread which can cause the new client to be used when
the old thread accepts a connection; fix by first stopping/interrupting/joining
the existing listenerThread (reference listenerThread and JDWPListenerThread)
and only after it has terminated assign the new _listenerState =
ListenerState(...) and start a new JDWPListenerThread, or alternatively change
the accept callback (onConnectedToVm) to receive the accepted ListenerState
instance instead of reading the mutable _listenerState field so the accepted
connection uses the correct ListenerState/client.
---
Duplicate comments:
In
`@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt`:
- Around line 136-145: The unregister method currently throws when
mRegister.remove(serverId) returns null; change it to be a no-op on missing
servers by returning early if the removed value is null. In the
lock.writeLock().withLock block call mRegister.remove(serverId) into registered
and, immediately after the lock, if registered is null simply return; only call
registered.shutdown() inside the try/catch for a non-null registered instance
(keeping the existing try/catch around registered.shutdown()) so teardown is
idempotent.
- Around line 62-78: The loop in DefaultLanguageServerRegistry.kt currently
catches Throwable and converts fatal JVM errors into per-server Failure; change
the catch to catch Exception instead of Throwable (but keep the existing check
that rethrows CancellationException) so we only handle normal exceptions from
server.connectDebugClient and still rethrow CancellationException; update the
catch block that logs via sLogger.error for server.serverId and assigns
DebugClientConnectionResult.Failure(cause = e) to use the Exception type.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/ListenerState.kt`:
- Around line 48-53: The invalidate() method flips invalidated after
stopListening(), which allows JDWPListenerThread.run() to see !isListening &&
!isInvalidated and restart the listener; change invalidate() to set
invalidated.set(true) before calling stopListening() so the run loop sees the
invalidated flag immediately and will not reopen the listener during
shutdown/replacement (refer to invalidate(), invalidated, stopListening() and
JDWPListenerThread.run()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d277436-f07a-456e-9928-24d4d23cd4e6
📒 Files selected for processing (7)
app/src/main/java/com/itsaky/androidide/actions/build/DebugAction.ktapp/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServer.ktlsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServerRegistry.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/ListenerState.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- lsp/api/src/main/java/com/itsaky/androidide/lsp/api/ILanguageServer.kt
- app/src/main/java/com/itsaky/androidide/actions/build/DebugAction.kt
Comment on lines
+986
to
+1025
| if (results.any { it is DebugClientConnectionResult.Failure }) { | ||
| // one or more debug adapters failed to initialize | ||
| val message = buildString { | ||
| results.filterIsInstance<DebugClientConnectionResult.Failure>().forEach { result -> | ||
| val msg = result.contextRes?.let(::getString) | ||
| ?: result.context | ||
| ?: (result.cause as? SocketException?).let { err -> | ||
| val msg = err?.message ?: "" | ||
| when { | ||
| msg.contains("EPERM") -> getString(string.debugger_error_errno_eperm) | ||
| msg.contains("ECONNREFUSED") -> getString(string.debugger_error_errno_econnrefused) | ||
| else -> null | ||
| } | ||
| } | ||
| ?: (result.cause as? ErrnoException? ?: result.cause?.cause as? ErrnoException?)?.let { err -> | ||
| when (err.errno) { | ||
| OsConstants.EPERM -> getString(string.debugger_error_errno_eperm) | ||
| OsConstants.ECONNREFUSED -> getString(string.debugger_error_errno_econnrefused) | ||
| else -> getString(R.string.debugger_error_errno, err.errno) | ||
| } | ||
| } | ||
| ?: getString(R.string.debugger_error_debugger_startup_failure) | ||
|
|
||
| append(msg) | ||
| append(System.lineSeparator()) | ||
| } | ||
|
|
||
| if (isNotBlank()) { | ||
| append(System.lineSeparator()) | ||
| } | ||
|
|
||
| append(getString(R.string.debugger_error_suggestion_network_restriction)) | ||
| } | ||
|
|
||
| withContext(Dispatchers.Main) { | ||
| newMaterialDialogBuilder(this@ProjectHandlerActivity) | ||
| .setTitle(R.string.debugger_error_network_access_error) | ||
| .setMessage(message) | ||
| .setPositiveButton(android.R.string.ok, null) | ||
| .show() |
Contributor
There was a problem hiding this comment.
Don't show the network-access dialog for every debug startup failure.
This branch always uses the network-access title and appends the restriction suggestion, even when the classifier falls back to the generic startup message on Line 1007. That will misdiagnose non-network failures as local-network denial. Only use the network-specific copy when one of the failures was actually identified as EPERM / network-related.
Comment on lines
+83
to
+84
| override fun destroy() { | ||
| EventBus.getDefault().unregister(this) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt | head -150Repository: appdevforall/CodeOnTheGo
Length of output: 5402
🏁 Script executed:
# Let's also search for the register and destroy methods more specifically
rg -A 10 "override fun destroy" lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.ktRepository: appdevforall/CodeOnTheGo
Length of output: 377
🏁 Script executed:
rg -A 15 "override fun register\(server: ILanguageServer\)" lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.ktRepository: appdevforall/CodeOnTheGo
Length of output: 469
Make the EventBus subscription lifecycle idempotent and thread-safe.
destroy() always calls unregister(this), which throws IllegalArgumentException if this registry was never subscribed. Additionally, register() performs isRegistered() and register(this) outside the write lock. On greenrobot EventBus, this creates a race condition: two concurrent register() calls can both observe isRegistered() as false before either executes registration, violating the thread-safety contract. Guard unregister() with isRegistered() and move the subscription check into the critical section alongside the map update.
Suggested fix
override fun destroy() {
- EventBus.getDefault().unregister(this)
+ EventBus.getDefault().let { bus ->
+ if (bus.isRegistered(this)) {
+ bus.unregister(this)
+ }
+ }
val servers = lock.readLock().withLock { mRegister.values.toList() }
for (server in servers) {
try {
@@
override fun register(server: ILanguageServer) {
- if (!EventBus.getDefault().isRegistered(this)) {
- EventBus.getDefault().register(this)
- }
-
lock.writeLock().lock()
try {
+ val bus = EventBus.getDefault()
+ if (!bus.isRegistered(this)) {
+ bus.register(this)
+ }
val old = mRegister.putIfAbsent(server.serverId, server)
if (old != null) {
sLogger.warn("Attempt to re-register LSP server with ID '{}'", server.serverId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lsp/api/src/main/java/com/itsaky/androidide/lsp/api/DefaultLanguageServerRegistry.kt`
around lines 83 - 84, Make the EventBus subscription lifecycle idempotent and
thread-safe by (1) guarding EventBus.getDefault().unregister(this) inside
destroy() with EventBus.getDefault().isRegistered(this) so unregister is only
called when subscribed, and (2) moving the isRegistered() check currently in
register() into the same write-lock critical section where the registry map is
updated so registration and the map update occur atomically; update the
register() method to acquire the write lock, check isRegistered(), then call
register(this) and update the map only if not already registered.
Comment on lines
+148
to
178
| _listenerState?.invalidate() | ||
| _listenerState = | ||
| ListenerState( | ||
| client = client, | ||
| connector = connector, | ||
| args = args, | ||
| ) | ||
|
|
||
| this.listenerThread = | ||
| 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) | ||
| } | ||
| } | ||
|
|
||
| if (failure != null) { | ||
| return failure | ||
| } | ||
|
|
||
| listenerThread = | ||
| JDWPListenerThread( | ||
| _listenerState!!, | ||
| this::onConnectedToVm, | ||
| ).also { thread -> thread.start() } |
Contributor
There was a problem hiding this comment.
Stop the previous listener thread before replacing _listenerState.
_listenerState is reassigned here before the old JDWPListenerThread is interrupted or joined. If that thread completes an accept() in the meantime, onConnectedToVm() later reads _listenerState!!.client (Line 239) and can attach the VM to the new debug client instead of the one that accepted it. Tear down the old thread first, or pass the accepted ListenerState through the callback instead of reading the mutable field later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`
around lines 148 - 178, Currently you reassign _listenerState before tearing
down the previous JDWPListenerThread which can cause the new client to be used
when the old thread accepts a connection; fix by first
stopping/interrupting/joining the existing listenerThread (reference
listenerThread and JDWPListenerThread) and only after it has terminated assign
the new _listenerState = ListenerState(...) and start a new JDWPListenerThread,
or alternatively change the accept callback (onConnectedToVm) to receive the
accepted ListenerState instance instead of reading the mutable _listenerState
field so the accepted connection uses the correct ListenerState/client.
jatezzz
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See ADFA-3191 for more details.