[
https://issues.apache.org/jira/browse/JAMES-3724?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Benoit Tellier closed JAMES-3724.
---------------------------------
Fix Version/s: 3.8.0
Resolution: Fixed
We now have a leak detector!
> Lifecycle API should embed a leak detector
> ------------------------------------------
>
> Key: JAMES-3724
> URL: https://issues.apache.org/jira/browse/JAMES-3724
> Project: James Server
> Issue Type: Improvement
> Components: James Core
> Affects Versions: 3.7.0
> Reporter: Benoit Tellier
> Priority: Major
> Fix For: 3.8.0
>
> Time Spent: 4h 40m
> Remaining Estimate: 0h
>
> h3. Why ?
> JAMES-3720 showed how easy to miss calling Lifecycle::dispose on email
> objects, which causes potentially temporary file leaks. If core developer /
> core components are subject to leaks while core contributors are aware of the
> importance of life-cycle management of mail objects, I cannot but be worry of
> users writing extensions and messing up.
> h3. Goals
> Thus I would like a defense mechanism that:
> - Log errors if an email is not disposed, and ideally contains a stacktrace
> to where that email was created to ease debug.
> - GC should eventually reclaim those temporary files, this is better than
> leaking them.
> h3. Solution overview
> I have in mind something much similar to Netty 4 pooled buffer leak detection:
> - Rely on phantom references to track GCed items
> - Fill a stack trace upon object creation
> - Upon GC verify that resources were released, if not do it and warn the
> user about it.
> References:
> - About phantom references: https://www.baeldung.com/java-phantom-reference
> - Netty resource leak detector:
> https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/ResourceLeakDetector.java
> h3. API overview
> Use system properties to configure leak detection:
> - `-Djames.ligecycle.leak.detection.mode=none` would behave as today.
> - `-Djames.ligecycle.leak.detection.mode=simple` would output a simple ERROR
> log if a leak is encountered and would free the resources.
> - `-Djames.ligecycle.leak.detection.mode=advanced` would output an extented
> ERROR log implying the place of allocation of the underlying object and would
> free resources.
> - `-Djames.ligecycle.leak.detection.mode=testing` would output an extented
> ERROR log implying the place of allocation of the underlying object and
> rethrow an error, unsuring test fails and that action is being taken by the
> development team.
> `jvm.properties` files and documentation would document this setting.
> Also we IMO should default to `simple`.
> h3. Implementation details
> Provide an abstract class, Disposable.LeakAware:
> {code:java}
> public interface Disposable {
> /**
> * Dispose the object
> */
> void dispose();
> static abstract class LeakAware implements Disposable {
> private static final ReferenceQueue<Object> REFERENCE_QUEUE = new
> ReferenceQueue<>();
> private Atomic<Boolean> isDisposed:
> protected LeakAware() {
> isDisposed = new AtomicBoolean(false);
> }
> public void track() {
> // Create a phantom reference, of course if detection mode is
> none
> new LeakAwareFinalizer(this, REFERENCE_QUEUE)
> // POLL the reference queue here to check for leaks!
> // of course don't do it if the leak detection mode is none
> while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
> ((LeakAwareFinalizer)referenceFromQueue).detectLeak();
> referenceFromQueue.clear();
> }
> }
> }
> public class LeakAwareFinalizer extends PhantomReference<LeakAware> {
> public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super
> LeakAware> q) {
> super(referent, q);
> }
> public void detectLeak() {
> if (!referent.isDisposed()) {
> // add starttrace of creation if needed
> LOGGER.error("Leak detected!!!");
> // clean if needed
> referent.dispose();
> }
> }
> }
> }
> {code}
> Then sensitive classes, likely only `MimeMessageInputStreamSource` would
> extend it, and thus automatically gain leak detection.
> We can turn `MimeMessageSource` into an interface with a default size method
> if needed.
> h3. Acceptance criteria
> - Run tests in testing leak detection mode. This can be done by passing
> system properties to the surefire plugin.
> - Unit tests including logging checks for each mode.
> - See this for logging assertions: PeriodicalHealthCheckTest
> - Call System.gc() to force a GC and get the leak detection running
> - Manage dynamically system variables in unit tests to test each mode.
> - Verify release was called (via atomic boolean in a custom
> Disposable.LeakAware object?)
> - Document this feature.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org
