tabish121 commented on code in PR #4418:
URL: https://github.com/apache/activemq-artemis/pull/4418#discussion_r1151224880
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -49,6 +76,28 @@ public int getDurableCount() {
return DURABLE_REF_COUNT_UPDATER.get(this);
}
+ /** Sub classes may mark fired=true when they were explicitly called.
+ * E.g large message removed the file upon cancellation */
+ protected void markFired() {
+ fired = true;
+ if (firedAtomic != null) {
+ firedAtomic.set(true);
+ }
+ }
+
+ public RefCountMessage() {
+ if (DEBUG_REF_COUNT) {
+ AtomicBoolean refFired = this.firedAtomic;
+ List<Exception> crumbs = debugCrumbs;
+ crumbs.add(new Exception("new Instance"));
+ String clazz = this.getClass().getName();
+ // notice you can't have a direct reference on the LargeMessage
Itself,
+ // Otherwise you would get a leak from itself
+ // RefCountMessageLeakTest is validating this scenario
+ CLEANER.register(this, () -> debug(clazz, refFired, crumbs));
Review Comment:
Even with the comment I'm not a huge fan of the lambda being used here
because it is super easy for someone to later change this to capture 'this'
inadvertently. I'd recommend passing the state data to a static function that
returns a Runnable just as an additional safety mechanism as that prevents a
capture of 'this' in the Runnable.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -58,54 +107,42 @@ public int getDurableCount() {
public RefCountMessage getParentRef() {
return parentRef;
}
- // I am usually against keeping commented out code
- // However this is very useful for me to debug referencing counting.
- // Uncomment out anything between #ifdef DEBUG and #endif
- // #ifdef DEBUG -- comment out anything before endif if you want to debug
REFERENCE COUNTS
- //final ConcurrentHashSet<Exception> upSet = new ConcurrentHashSet<>();
- // #endif
+ final ArrayList<Exception> debugCrumbs = DEBUG_REF_COUNT ? new
ArrayList<>() : null;
private void onUp() {
- // #ifdef DEBUG -- comment out anything before endif if you want to
debug REFERENCE COUNTS
- // upSet.add(new Exception("upEvent(" + debugString() + ")"));
- // #endif
+ if (DEBUG_REF_COUNT) {
+ debugCrumbs.add(new Exception("upEvent(" + debugString() + ")"));
+ }
}
private void onDown() {
- // #ifdef DEBUG -- comment out anything before endif if you want to
debug REFERENCE COUNTS
- // upSet.add(new Exception("upEvent(" + debugString() + ")"));
- // #endif
+ if (DEBUG_REF_COUNT) {
+ debugCrumbs.add(new Exception("DownEvent(" + debugString() + ")"));
+ }
if (getRefCount() <= 0 && getUsage() <= 0 && getDurableCount() <= 0 &&
!fired) {
-
- debugRefs();
fired = true;
releaseComplete();
}
}
- /**
- *
- * This method will be useful if you remove commented out code around
#ifdef AND #endif COMMENTS
- * */
- public final void debugRefs() {
- // #ifdef DEBUG -- comment out anything before endif if you want to
debug REFERENCE COUNTS
- // try {
- //
System.err.println("************************************************************************************************************************");
- // System.err.println("Printing refcounts for " + debugString() + "
this = " + this);
- // for (Exception e : upSet) {
- // e.printStackTrace();
- // }
- //
System.err.println("************************************************************************************************************************");
- // } catch (Throwable e) {
- // e.printStackTrace();
- // }
- // #ifdef DEBUG -- comment out anything before endif if you want to
debug REFERENCE COUNTS
- }
+ private static void debug(String clazz, AtomicBoolean firedAtomic,
List<Exception> crumbs) {
+ if (!firedAtomic.get()) {
+ StringWriter writer = new StringWriter();
+ PrintWriter out = new PrintWriter(writer);
+ crumbs.forEach(e -> e.printStackTrace(out));
+ ActiveMQClientLogger.LOGGER.debugMessageNotReleased(clazz,
writer.toString());
+ }
+ }
Review Comment:
Extra newline to remove.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,40 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.lang.ref.Cleaner;
+import java.util.ArrayList;
import java.util.HashMap;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
-// import org.apache.activemq.artemis.utils.collections.ConcurrentHashSet; --
#ifdef DEBUG
+import org.apache.activemq.artemis.core.client.ActiveMQClientLogger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class RefCountMessage {
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
Review Comment:
Extra newline to remove.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/Test.java:
##########
Review Comment:
This seems like an odd thing to be adding to the main jar for the core
client, surely you don't need to add test classes into the client ?
##########
tests/leak-tests/src/test/java/org/apache/activemq/artemis/tests/leak/RefCountMessageLeakTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.leak;
+
+import java.lang.invoke.MethodHandles;
+
+import io.github.checkleak.core.CheckLeak;
+import org.apache.activemq.artemis.api.core.RefCountMessage;
+import org.apache.activemq.artemis.logs.AssertionLoggerHandler;
+import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.apache.activemq.artemis.utils.Wait;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RefCountMessageLeakTest extends ActiveMQTestBase {
+
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ static class DebugMessage extends RefCountMessage {
+ final String string;
+
+ DebugMessage(String str) {
+ this.string = str;
+ }
+
+ @Override
+ public String toString() {
+ return "debugMessage(" + string + ")";
+ }
+
+ public void fired() {
+ this.markFired();
+ }
+
Review Comment:
Unnecessary extra newlines to remove.
--
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]
