Skip to content

feat: add opt-in resource leak detector to sdk-core#151

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/resource-leak-detector
Open

feat: add opt-in resource leak detector to sdk-core#151
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/resource-leak-detector

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

A caller who forgets to close a response body or stream gets no signal until something downstream breaks. This adds an opt-in, log-only detector that warns when a closeable resource becomes phantom-reachable without having been closed.

What it does

  • LeakDetector.track(resource, description) registers a resource and returns a LeakTracker; the resource calls LeakTracker.closed() from its close() to mark a clean shutdown.
  • LeakDetector.trackCloseable(closeable, description) wraps an AutoCloseable so close() auto-marks the tracker. This is the by-hand integration point a response-body or stream factory would use today.
  • When a tracked resource is reclaimed without having been closed, the detector emits a LeakReport — by default a single WARN log line via SLF4J, optionally carrying the creation stack.

What it deliberately does NOT do

It never closes anything. Auto-closing a caller-owned resource from a GC callback is unsafe (arbitrary thread, arbitrary time, transport state the caller may still own), so detection is strictly observational. Program behavior is identical whether the detector is on or off.

Off by default

LeakDetector.systemDefault reads dexpace.sdk.leakDetection and is disabled unless set to true (creation-stack capture is gated separately by dexpace.sdk.leakDetection.captureStack). A Builder lets an application enable it explicitly and plug in a custom LeakListener.

Mechanism (Java 8 compatible)

Detection uses one PhantomReference per tracked resource plus a ReferenceQueue drained by a single daemon reaper thread. This works identically on every JDK from 8 up and needs no java.lang.ref.Cleaner (which is 9+). A tracked resource shares an AtomicBoolean with its tracker, so a closed resource is never reported.

Testing

drainManually() processes already-enqueued leaks synchronously, so tests drive detection with the reaper thread disabled and assert race-free after a bounded GC-poll loop (polls System.gc() against a WeakReference sentinel, fails fast rather than flaking if the collector never runs). Coverage includes: unclosed resource reported, closed resource not reported, idempotent close, disabled-path no-op, creation-stack on/off, a throwing listener not stalling the drain, the trackCloseable wrapper paths, and systemDefault being off without the property.

Gated build commands run (all green)

./gradlew :sdk-core:test --tests "org.dexpace.sdk.core.lifecycle.LeakDetectorTest" \
  :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck --no-daemon
./gradlew :sdk-core:test --no-daemon   # full sdk-core suite
./gradlew :sdk-core:apiDump --no-daemon # regenerated sdk-core.api committed

Closes #45

Add a log-only detector that warns when a closeable resource (e.g. a
response body) becomes phantom-reachable without having been closed.

The detector is off by default and never changes program behavior: it
only observes. Auto-closing a caller-owned resource from a GC callback is
unsafe, so detection is strictly diagnostic.

- LeakDetector tracks resources via a PhantomReference per resource plus a
  ReferenceQueue drained by a single daemon reaper thread. This works on
  every JDK from 8 up and needs no java.lang.ref.Cleaner (9+).
- A tracked resource shares an AtomicBoolean with its LeakTracker;
  LeakTracker.closed() flips the flag and de-registers the phantom, so a
  cleanly-closed resource is never reported.
- trackCloseable wraps an AutoCloseable so close() auto-marks the tracker,
  the by-hand integration point a response-body factory uses today.
- systemDefault reads dexpace.sdk.leakDetection (and .captureStack); a
  Builder lets applications enable it explicitly and supply a custom
  LeakListener (default logs each leak at WARN via SLF4J).
- drainManually() processes already-enqueued leaks synchronously, giving
  tests a race-free trigger after a GC-poll loop.

Closes #45
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds an opt-in, log-only resource leak detector under a new org.dexpace.sdk.core.lifecycle package, tracking AutoCloseable/arbitrary resources via a PhantomReference + ReferenceQueue drained by a single daemon reaper thread. It's disabled by default, never auto-closes anything, and exposes drainManually() for deterministic tests. The design is sound and Java 8 compatible, and the api snapshot is regenerated correctly. Good to merge after a couple of small follow-ups.

Issues

  • New lifecycle package missing from README package map (README.md:275-279). The package table documents sibling packages like pagination and instrumentation, but the new public org.dexpace.sdk.core.lifecycle package with its five public types has no row. CLAUDE.md treats the README as the canonical full package map, so it should be added to keep that map complete.

  • Reaper thread loop has no test coverage (sdk-core/src/main/kotlin/org/dexpace/sdk/core/lifecycle/LeakDetector.kt:231-252). Every test runs with startReaperThread(false) and drives detection via drainManually(), so the live runtime path — startReaper(), the queue.remove() blocking loop, the InterruptedException interrupt-restore handling, and the lazy double-start guard — is never exercised. That's the path that matters most in production usage; worth a test that actually starts the thread.

  • creationStack getter exposes the internal array without a defensive copy (sdk-core/src/main/kotlin/org/dexpace/sdk/core/lifecycle/LeakReport.kt:473-476). LeakReport stores the captured StackTraceElement[] by reference and the generated getter returns it directly, so a listener could mutate the report's stack array. Throwable.getStackTrace() clones for exactly this reason. Minor, since StackTraceElement is immutable and the array is short-lived, but cloning on the way out would close the gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Opt-in resource leak detector (log-only, no auto-close)

1 participant