chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823582462
##########
File path:
server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -119,49 +129,75 @@ public T getResource() {
}
}
- class LeakAwareFinalizer extends PhantomReference<LeakAware> {
+ class TraceRecord {
+ private final List<StackWalker.StackFrame> stackFrames;
+
+ TraceRecord(List<StackWalker.StackFrame> stackFrames) {
+ this.stackFrames = stackFrames;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder buf = new StringBuilder();
+ this.stackFrames.subList(3, this.stackFrames.size())
+ .forEach(stackFrame -> {
+ buf.append("\t");
+ buf.append(stackFrame.getClassName());
+ buf.append("#");
+ buf.append(stackFrame.getMethodName());
+ buf.append(":");
+ buf.append(stackFrame.getLineNumber());
+ buf.append("\n");
+ });
+ return buf.toString();
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<LeakAware<?>> {
private static final Logger LOGGER =
LoggerFactory.getLogger(LeakAwareFinalizer.class);
private final LeakAware.Resource resource;
+ private TraceRecord traceRecord;
- public LeakAwareFinalizer(LeakAware referent, LeakAware.Resource
resource, ReferenceQueue<? super LeakAware> q) {
+ public LeakAwareFinalizer(LeakAware referent, LeakAware.Resource
resource, ReferenceQueue<? super LeakAware<?>> q) {
super(referent, q);
this.resource = resource;
+ if (LeakAware.tracedEnabled()) {
+ traceRecord = new TraceRecord(StackWalker.getInstance().walk(s
-> s.collect(Collectors.toList())));
+ }
}
public void detectLeak() {
switch (LeakAware.LEVEL) {
case NONE: // nothing
break;
- case SIMPLE: {
- if (isNotDisposed()) {
- LOGGER.error("Leak detected! Resource {} was not
released before its referent was GCed. " +
- "Resource management needs to be reviewed: ensure
to always call dispose() for disposable objects you work with. " +
- "Consider enabling advanced leak detection to
further identify the problem.", resource);
- resource.dispose();
- LeakAware.REFERENCES_IN_USE.remove(this);
- }
- break;
- }
+ case SIMPLE:
case ADVANCED: {
if (isNotDisposed()) {
- extendedErrorLog();
+ errorLog();
resource.dispose();
LeakAware.REFERENCES_IN_USE.remove(this);
}
break;
}
case TESTING: {
if (isNotDisposed()) {
- extendedErrorLog();
+ errorLog();
throw new LeakAware.LeakDetectorException();
}
}
}
}
- public void extendedErrorLog() {
- LOGGER.error("Extended error log!!!");
+ public void errorLog() {
+ if (LeakAware.tracedEnabled()) {
+ LOGGER.error("Leak detected! Resource {} was not released
before its referent was garbage-collected. \n" +
+ "Trace record: \n{}", resource, traceRecord.toString());
Review comment:
`Trace record:` -> `This resource was instanciated at:`
##########
File path:
server/container/lifecycle-api/src/test/java/org/apache/james/lifecycle/api/LeakAwareTest.java
##########
@@ -20,66 +20,180 @@
package org.apache.james.lifecycle.api;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.awaitility.Durations.ONE_HUNDRED_MILLISECONDS;
+import static org.awaitility.Durations.TEN_SECONDS;
+import static org.apache.james.lifecycle.api.Disposable.LeakAware;
-import java.util.concurrent.TimeUnit;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
import java.util.concurrent.atomic.AtomicBoolean;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionFactory;
+
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.LoggerFactory;
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
class LeakAwareTest {
- private static final class LeakResourceSample extends Disposable.LeakAware
implements Disposable {
- public static LeakResourceSample create(AtomicBoolean atomicBoolean) {
- return new LeakResourceSample(new Resource(() ->
atomicBoolean.set(true)), atomicBoolean);
+ private static final class LeakResourceSample extends
LeakAware<LeakResourceSample.TestResource> {
+ static class TestResource extends LeakAware.Resource {
+ public TestResource(Disposable cleanup) {
+ super(cleanup);
+ }
}
- private final AtomicBoolean isDisposed;
+ public static LeakResourceSample create(AtomicBoolean atomicBoolean) {
+ return new LeakResourceSample(new TestResource(() ->
atomicBoolean.set(true)));
+ }
- private LeakResourceSample(Resource resource, AtomicBoolean
isDisposed) {
+ LeakResourceSample(TestResource resource) {
super(resource);
- this.isDisposed = isDisposed;
}
+ }
- // Do something with the boolean
- boolean isTrue() {
- return isDisposed.get();
- }
+ private static final ConditionFactory awaitAtMostTenSeconds = Awaitility
+ .with().pollInterval(ONE_HUNDRED_MILLISECONDS)
+ .and().pollDelay(ONE_HUNDRED_MILLISECONDS)
+ .await()
+ .atMost(TEN_SECONDS);
+
+ public static ListAppender<ILoggingEvent> getListAppenderForClass(Class
clazz) {
+ Logger logger = (Logger) LoggerFactory.getLogger(clazz);
+
+ ListAppender<ILoggingEvent> loggingEventListAppender = new
ListAppender<>();
+ loggingEventListAppender.start();
+
+ logger.addAppender(loggingEventListAppender);
+ return loggingEventListAppender;
+ }
+ private void forceChangeLevel(String level) throws NoSuchFieldException,
IllegalAccessException {
Review comment:
missing line break
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
