[
https://issues.apache.org/jira/browse/JAMES-3724?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Benoit Tellier updated JAMES-3724:
----------------------------------
Description:
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.
was:
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:
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.
> 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
>
> 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: [email protected]
For additional commands, e-mail: [email protected]
