You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2022/03/09 10:05:40 UTC
[GitHub] [james-project] vttranlina opened a new pull request #911: JAMES-3724 - Implement LeakAware
vttranlina opened a new pull request #911:
URL: https://github.com/apache/james-project/pull/911
Jira: https://issues.apache.org/jira/browse/JAMES-3724
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823363895
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
Review comment:
> We can use Collections.synchronizedSet to replace it.
Please no synchronize. ConcurrentHashMap for the win!
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823344871
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +38,134 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware<T extends LeakAware.Resource> implements Disposable {
+ public static class Resource implements Disposable {
+ private final AtomicBoolean isDisposed = new AtomicBoolean(false);
+ private final Disposable cleanup;
+
+ public Resource(Disposable cleanup) {
+ this.cleanup = cleanup;
+ }
+
+ public boolean isDisposed() {
+ return isDisposed.get();
+ }
+
+ @Override
+ public void dispose() {
+ isDisposed.set(true);
+ cleanup.dispose();
+ }
+ }
+
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<LeakAware> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final ConcurrentHashMap<LeakAwareFinalizer, Boolean> REFERENCES_IN_USE = new ConcurrentHashMap<>();
+ public static final Level LEVEL = Level.SIMPLE;
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return LEVEL != Level.NONE;
+ }
+
+ private final T resource;
+ private final LeakAwareFinalizer finalizer;
+
+ protected LeakAware(T resource) {
+ this.resource = resource;
+ this.finalizer = new LeakAwareFinalizer(this, resource, REFERENCE_QUEUE);
+ REFERENCES_IN_USE.put(finalizer, true);
+ }
+
+ @Override
+ public void dispose() {
+ REFERENCES_IN_USE.remove(finalizer);
+ resource.dispose();
+ }
+
+ public T getResource() {
+ return resource;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<LeakAware> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private final LeakAware.Resource resource;
+
+ public LeakAwareFinalizer(LeakAware referent, LeakAware.Resource resource, ReferenceQueue<? super LeakAware> q) {
+ super(referent, q);
+ this.resource = resource;
Review comment:
With this pattern, we will only handler `resource.dispose()`, we can miss `LeakAware.dispose`
Example `org.apache.james.server.core.MailImpl#dispose`
Is this a problem?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822689199
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
Maybe we need one more object, that will wrap each resource.
I'm try it
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822697791
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
The cleanup needs to be a callback to the reference cf https://dzone.com/articles/weak-soft-and-phantom-references-in-java-and-why-they-matter
Since Phantom doesn't have a link to the actual object, a typical pattern is to derive your own Reference type from Phantom and add some info useful for the final freeing, for example filename.
LeakAware likely need to provide a callback to clean its guts, of course without referencing itself.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822690406
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
Ah ok I did not read the javadoc well enough
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822687712
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
`PhantomReference::get` always return NULL
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822487524
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
I'm stuck here,
`referent` is used by `isNotDisposed` method.
But if we set it, GC won't remove LeakAware from memory (REFERENCE_QUEUE always empty)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822768610
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
Review comment:
> BTW array list is not thread safe and should not be used in a threaded environment (which we have)
We can use `Collections.synchronizedSet` to replace it.
> What is that list used for?
> My feeling is that it is useless.
Yes, I'm confused here too, but it is necessary. It needs a collection to add `LeakAwareFinalizer`, if NOT, phantomReference mechanism will not work. (I learn this code from some samples)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823479850
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
Review comment:
Why grouping those new classes inside the Disposable interface and not extracting them separately?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r824478744
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/jvm.adoc
##########
@@ -7,3 +7,15 @@ Note that in some rare cases this might not work,
when a property affects very early JVM start behaviour.
For testing purposes, you may specify a different file path via the command line option `-Dextra.props=/some/other/jvm.properties`.
+
+== Running resource leak detection
Review comment:
How about `james.message.memory.threshold` and `james.message.usememorycopy` ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822674306
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
Review comment:
Missing a final qualifier IMO
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
Review comment:
Would it be possible to specify a more precise type than "object" here?
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
Review comment:
What is that list used for?
My feeling is that it is useless.
BTW array list is not thread safe and should not be used in a threaded environment (which we have)
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
Review comment:
I prefer initialiwation of this field to be done in a static field so that it's evaluated only once at APP startup.
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
+ }
+
+ public void detectLeak() {
+ switch (LeakAware.level) {
+ case NONE: // nothing
+ break;
+ case SIMPLE: {
+ if (isNotDisposed()) {
+ LOGGER.error("Leak detected!!!");
Review comment:
Log at least the object for which it is detected.
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
Review comment:
static things should be sorted first.
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
That's the finalizer that get gced and that should thus not be referenced. That's the finalizer that should be in the queue.
From what I understood.
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
+ }
+
+ public void detectLeak() {
+ switch (LeakAware.level) {
+ case NONE: // nothing
+ break;
+ case SIMPLE: {
+ if (isNotDisposed()) {
+ LOGGER.error("Leak detected!!!");
Review comment:
Even with simple we should dispore the referent...
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
Review comment:
Are those constructors used?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r825559125
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +41,169 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware<T extends LeakAware.Resource> implements Disposable {
+ public static class Resource implements Disposable {
+ private final AtomicBoolean isDisposed = new AtomicBoolean(false);
+ private final Disposable cleanup;
+
+ public Resource(Disposable cleanup) {
+ this.cleanup = cleanup;
+ }
+
+ public boolean isDisposed() {
+ return isDisposed.get();
+ }
+
+ @Override
+ public void dispose() {
+ isDisposed.set(true);
+ cleanup.dispose();
+ }
+ }
+
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<LeakAware<?>> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final ConcurrentHashMap<LeakAwareFinalizer, Boolean> REFERENCES_IN_USE = new ConcurrentHashMap<>();
+ public static final Level LEVEL = Optional.ofNullable(System.getProperty("james.ligecycle.leak.detection.mode"))
Review comment:
```suggestion
public static final Level LEVEL = Optional.ofNullable(System.getProperty("james.lifecycle.leak.detection.mode"))
```
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +41,169 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware<T extends LeakAware.Resource> implements Disposable {
+ public static class Resource implements Disposable {
+ private final AtomicBoolean isDisposed = new AtomicBoolean(false);
+ private final Disposable cleanup;
+
+ public Resource(Disposable cleanup) {
+ this.cleanup = cleanup;
+ }
+
+ public boolean isDisposed() {
+ return isDisposed.get();
+ }
+
+ @Override
+ public void dispose() {
+ isDisposed.set(true);
+ cleanup.dispose();
+ }
+ }
+
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<LeakAware<?>> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final ConcurrentHashMap<LeakAwareFinalizer, Boolean> REFERENCES_IN_USE = new ConcurrentHashMap<>();
+ public static final Level LEVEL = Optional.ofNullable(System.getProperty("james.ligecycle.leak.detection.mode"))
Review comment:
And change the rest of the code accordingly :)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822686547
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
https://docs.oracle.com/javase/8/docs/api/java/lang/ref/PhantomReference.html#get--
Use `PhantomReference::get` and don't store the reference yourself.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823364273
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +38,134 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware<T extends LeakAware.Resource> implements Disposable {
+ public static class Resource implements Disposable {
+ private final AtomicBoolean isDisposed = new AtomicBoolean(false);
+ private final Disposable cleanup;
+
+ public Resource(Disposable cleanup) {
+ this.cleanup = cleanup;
+ }
+
+ public boolean isDisposed() {
+ return isDisposed.get();
+ }
+
+ @Override
+ public void dispose() {
+ isDisposed.set(true);
+ cleanup.dispose();
+ }
+ }
+
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<LeakAware> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final ConcurrentHashMap<LeakAwareFinalizer, Boolean> REFERENCES_IN_USE = new ConcurrentHashMap<>();
+ public static final Level LEVEL = Level.SIMPLE;
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return LEVEL != Level.NONE;
+ }
+
+ private final T resource;
+ private final LeakAwareFinalizer finalizer;
+
+ protected LeakAware(T resource) {
+ this.resource = resource;
+ this.finalizer = new LeakAwareFinalizer(this, resource, REFERENCE_QUEUE);
+ REFERENCES_IN_USE.put(finalizer, true);
+ }
+
+ @Override
+ public void dispose() {
+ REFERENCES_IN_USE.remove(finalizer);
+ resource.dispose();
+ }
+
+ public T getResource() {
+ return resource;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<LeakAware> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private final LeakAware.Resource resource;
+
+ public LeakAwareFinalizer(LeakAware referent, LeakAware.Resource resource, ReferenceQueue<? super LeakAware> q) {
+ super(referent, q);
+ this.resource = resource;
Review comment:
No that's itended, leaks on MailImpl is notan issue, we don't need detection for it as it do not hold resources in itself.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r822701921
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
+ public static class LeakDetectorException extends RuntimeException {
+ public LeakDetectorException() {
+ super();
+ }
+
+ public LeakDetectorException(String message) {
+ super(message);
+ }
+
+ public LeakDetectorException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ public enum Level {
+ NONE,
+ SIMPLE,
+ ADVANCED,
+ TESTING;
+
+ static Level parse(String input) {
+ for (Level level : values()) {
+ if (level.name().equalsIgnoreCase(input)) {
+ return level;
+ }
+ }
+ throw new IllegalArgumentException(String.format("Unknown level `%s`", input));
+ }
+ }
+
+ public static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<>();
+ public static final List<LeakAwareFinalizer> LEAK_REFERENCES = new ArrayList<>();
+ public static Level level;
+ private final AtomicBoolean isDisposed;
+
+ protected LeakAware() {
+ isDisposed = new AtomicBoolean(false);
+ level = Level.SIMPLE; // todo
+ LEAK_REFERENCES.add(new LeakAwareFinalizer(this, REFERENCE_QUEUE));
+ }
+
+ public void disposed() {
+ isDisposed.set(true);
+ }
+
+ public static void track() {
+ if (leakDetectorEnabled()) {
+ Reference<?> referenceFromQueue;
+ while ((referenceFromQueue = REFERENCE_QUEUE.poll()) != null) {
+ ((LeakAwareFinalizer) referenceFromQueue).detectLeak();
+ referenceFromQueue.clear();
+ }
+ }
+ }
+
+ private static boolean leakDetectorEnabled() {
+ return level != Level.NONE;
+ }
+ }
+
+ class LeakAwareFinalizer extends PhantomReference<Object> {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LeakAwareFinalizer.class);
+
+ private LeakAware referent;
+
+ public LeakAwareFinalizer(LeakAware referent, ReferenceQueue<? super Object> q) {
+ super(referent, q);
+// this.referent = referent;
Review comment:
https://github.com/apache/commons-io/blob/aad6583bd2c18da84dd85bd61197c578febf388f/src/main/java/org/apache/commons/io/FileCleaningTracker.java#L84
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #911:
URL: https://github.com/apache/james-project/pull/911#discussion_r823507119
##########
File path: server/container/lifecycle-api/src/main/java/org/apache/james/lifecycle/api/Disposable.java
##########
@@ -29,4 +39,111 @@
* Dispose the object
*/
void dispose();
+
+ abstract class LeakAware implements Disposable {
Review comment:
IMO it make sense from a semantic point of view : as a caller I would type `Disposeable.LeakAware` which is very explicit. IMO with separated class you loose such a reading.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
vttranlina commented on pull request #911:
URL: https://github.com/apache/james-project/pull/911#issuecomment-1067472607
rebase master & squash fixup
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael merged pull request #911: JAMES-3724 - Implement LeakAware
Posted by GitBox <gi...@apache.org>.
Arsnael merged pull request #911:
URL: https://github.com/apache/james-project/pull/911
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org