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 14:07:16 UTC
[GitHub] [james-project] chibenwa commented on a change in pull request #911: JAMES-3724 - Implement LeakAware
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