gemmellr commented on code in PR #4418:
URL: https://github.com/apache/activemq-artemis/pull/4418#discussion_r1158669203
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,102 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.GregorianCalendar;
import java.util.HashMap;
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.apache.activemq.artemis.utils.ObjectCleaner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.helpers.MessageFormatter;
public class RefCountMessage {
+ private static final ThreadLocal<GregorianCalendar> threadLocalCalendar =
ThreadLocal.withInitial(() -> new GregorianCalendar());
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static boolean isDebugEnabled() {
+ return logger.isDebugEnabled();
+ }
+
+ public static boolean isTraceEnabled() {
+ return logger.isTraceEnabled();
+ }
+
+
+ private static class DebugState implements Runnable {
+
+ DebugState(String description) {
+ this.description = description;
+ debugCrumbs.add(new Exception("registered"));
Review Comment:
All the others have timestamps, seems weird this doesnt.
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/interruptlm/LargeMessageFrozenTest.java:
##########
@@ -0,0 +1,386 @@
+/*
+ * 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.soak.interruptlm;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Queue;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Locale;
+
+import org.apache.activemq.artemis.api.core.ActiveMQException;
+import org.apache.activemq.artemis.api.core.QueueConfiguration;
+import org.apache.activemq.artemis.api.core.RoutingType;
+import org.apache.activemq.artemis.core.server.ActiveMQServer;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl;
+import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory;
+import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
+import org.apache.activemq.artemis.tests.util.CFUtil;
+import org.apache.activemq.artemis.tests.util.TcpProxy;
+import org.apache.activemq.artemis.tests.util.Wait;
+import org.apache.qpid.jms.JmsConnectionFactory;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Test various scenarios with broker communication in large message */
+public class LargeMessageFrozenTest extends ActiveMQTestBase {
+
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ TcpProxy proxy;
+
+ ActiveMQServer server;
+
+ @Before
+ public void startServer() throws Exception {
+ server = createServer(true, true);
+ server.getConfiguration().addAcceptorConfiguration("alternate",
"tcp://localhost:44444?amqpIdleTimeout=100");
Review Comment:
100ms is very low (will advertise 50ms, cause client to heartbeat every 25).
I expect this could spuriously time out.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,102 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.GregorianCalendar;
import java.util.HashMap;
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.apache.activemq.artemis.utils.ObjectCleaner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.helpers.MessageFormatter;
public class RefCountMessage {
+ private static final ThreadLocal<GregorianCalendar> threadLocalCalendar =
ThreadLocal.withInitial(() -> new GregorianCalendar());
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static boolean isDebugEnabled() {
+ return logger.isDebugEnabled();
+ }
+
+ public static boolean isTraceEnabled() {
+ return logger.isTraceEnabled();
+ }
+
+
+ private static class DebugState implements Runnable {
+
+ DebugState(String description) {
+ this.description = description;
+ debugCrumbs.add(new Exception("registered"));
+ }
+
+ private final ArrayList<Exception> debugCrumbs = new ArrayList<>();
+
+ // this means the object is accounted for and it should not print any
warnings
+ volatile boolean accounted;
+
+ volatile boolean referenced;
+
+ String description;
+
+ /** To be called when leaked.
Review Comment:
'#To be called when leaked' seems superflous given its called
"runWhenLeaked". The other detail is important but relgated to another line.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,102 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.GregorianCalendar;
import java.util.HashMap;
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.apache.activemq.artemis.utils.ObjectCleaner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.helpers.MessageFormatter;
public class RefCountMessage {
+ private static final ThreadLocal<GregorianCalendar> threadLocalCalendar =
ThreadLocal.withInitial(() -> new GregorianCalendar());
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static boolean isDebugEnabled() {
+ return logger.isDebugEnabled();
+ }
+
+ public static boolean isTraceEnabled() {
+ return logger.isTraceEnabled();
+ }
+
+
+ private static class DebugState implements Runnable {
+
+ DebugState(String description) {
+ this.description = description;
+ debugCrumbs.add(new Exception("registered"));
+ }
+
+ private final ArrayList<Exception> debugCrumbs = new ArrayList<>();
+
+ // this means the object is accounted for and it should not print any
warnings
+ volatile boolean accounted;
+
+ volatile boolean referenced;
+
+ String description;
+
+ /** To be called when leaked.
+ * Notice: it can't hold any reference back to message otherwise it
won't ever happen and you will get a memory leak.*/
+ Runnable runWhenLeaked;
+
+ /** this marks the Status as accounted for
+ * and no need to report an issue when DEBUG hits */
+ void accountedFor() {
+ accounted = true;
+ }
+
+ public static String getTime() {
+ Calendar calendar = threadLocalCalendar.get();
+ calendar.setTimeInMillis(System.currentTimeMillis());
+ return String.format("%04d/%02d/%02d %02d:%02d:%02d:%03d",
calendar.get(Calendar.YEAR), calendar.get(Calendar.MONTH) + 1,
calendar.get(Calendar.DAY_OF_MONTH), calendar.get(Calendar.HOUR_OF_DAY),
calendar.get(Calendar.MINUTE), calendar.get(Calendar.SECOND),
calendar.get(Calendar.MILLISECOND));
Review Comment:
Perhaps simplify all this icky time stuff to something using Instant? E.g.
Instant.now().toString() ? May even be higher precision.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -1130,6 +1136,10 @@ public void addHead(final MessageReference ref, boolean
scheduling) {
/* Called when a message is cancelled back into the queue */
@Override
public void addSorted(final MessageReference ref, boolean scheduling) {
+
+ if (RefCountMessage.isTraceEnabled()) {
+ RefCountMessage.deferredDebug(ref.getMessage(), "returning to queue
{}", this.getAddress());
+ }
Review Comment:
The newline should be between the two entirely separate if's rather than
before them.
##########
artemis-unit-test-support/src/main/java/org/apache/activemq/artemis/utils/ThreadLeakCheckRule.java:
##########
@@ -284,8 +284,10 @@ private boolean isExpectedThread(Thread thread) {
} else if (threadName.contains("GC Daemon")) {
return true;
} else {
+ // validating for known stack traces
for (StackTraceElement element : thread.getStackTrace()) {
- if
(element.getClassName().contains("org.jboss.byteman.agent.TransformListener")) {
+ if
(element.getClassName().contains("org.jboss.byteman.agent.TransformListener") ||
+
element.getClassName().contains("dk.internal.ref.CleanerImpl")) {
Review Comment:
jdk?
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -58,55 +188,44 @@ 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
+ protected void onUp() {
+ if (debugStatus != null) {
+ debugStatus.up(counterString());
+ }
+ }
- private void onUp() {
- // #ifdef DEBUG -- comment out anything before endif if you want to
debug REFERENCE COUNTS
- // upSet.add(new Exception("upEvent(" + debugString() + ")"));
- // #endif
+ protected void released() {
+ released = true;
+ accountedFor();
}
- 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 (getRefCount() <= 0 && getUsage() <= 0 && getDurableCount() <= 0 &&
!fired) {
+ public void runOnLeak(Runnable run) {
+ if (debugStatus != null) {
+ debugStatus.runWhenLeaked = run;
+ }
+ }
Review Comment:
Only seems to be used from tests. Drop visibility, add an accessor?
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,102 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.GregorianCalendar;
import java.util.HashMap;
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.apache.activemq.artemis.utils.ObjectCleaner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.helpers.MessageFormatter;
public class RefCountMessage {
+ private static final ThreadLocal<GregorianCalendar> threadLocalCalendar =
ThreadLocal.withInitial(() -> new GregorianCalendar());
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static boolean isDebugEnabled() {
+ return logger.isDebugEnabled();
+ }
+
+ public static boolean isTraceEnabled() {
+ return logger.isTraceEnabled();
+ }
+
+
+ private static class DebugState implements Runnable {
+
+ DebugState(String description) {
Review Comment:
Odd having constructor before fields.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -16,13 +16,102 @@
*/
package org.apache.activemq.artemis.api.core;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.GregorianCalendar;
import java.util.HashMap;
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.apache.activemq.artemis.utils.ObjectCleaner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.helpers.MessageFormatter;
public class RefCountMessage {
+ private static final ThreadLocal<GregorianCalendar> threadLocalCalendar =
ThreadLocal.withInitial(() -> new GregorianCalendar());
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static boolean isDebugEnabled() {
+ return logger.isDebugEnabled();
+ }
+
+ public static boolean isTraceEnabled() {
+ return logger.isTraceEnabled();
+ }
+
+
+ private static class DebugState implements Runnable {
+
+ DebugState(String description) {
+ this.description = description;
+ debugCrumbs.add(new Exception("registered"));
+ }
+
+ private final ArrayList<Exception> debugCrumbs = new ArrayList<>();
+
+ // this means the object is accounted for and it should not print any
warnings
+ volatile boolean accounted;
+
+ volatile boolean referenced;
+
+ String description;
+
+ /** To be called when leaked.
+ * Notice: it can't hold any reference back to message otherwise it
won't ever happen and you will get a memory leak.*/
+ Runnable runWhenLeaked;
+
+ /** this marks the Status as accounted for
+ * and no need to report an issue when DEBUG hits */
+ void accountedFor() {
+ accounted = true;
+ }
+
+ public static String getTime() {
+ Calendar calendar = threadLocalCalendar.get();
+ calendar.setTimeInMillis(System.currentTimeMillis());
+ return String.format("%04d/%02d/%02d %02d:%02d:%02d:%03d",
calendar.get(Calendar.YEAR), calendar.get(Calendar.MONTH) + 1,
calendar.get(Calendar.DAY_OF_MONTH), calendar.get(Calendar.HOUR_OF_DAY),
calendar.get(Calendar.MINUTE), calendar.get(Calendar.SECOND),
calendar.get(Calendar.MILLISECOND));
+ }
+
+ void addDebug(String event) {
+ debugCrumbs.add(new Exception(event + " at " + getTime()));
+ if (accounted) {
+ logger.debug("Message Previously Released {}, {}, \n{}",
description, event, debugLocations());
+ }
+ }
+
+ void up(String description) {
+ referenced = true;
+ debugCrumbs.add(new Exception("UP:" + description + " at " +
getTime()));
+ }
+
+ void down(String description) {
+ debugCrumbs.add(new Exception("Down:" + description + " at " +
getTime()));
Review Comment:
Differing case between UP and Down.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java:
##########
@@ -49,6 +172,13 @@ public int getDurableCount() {
return DURABLE_REF_COUNT_UPDATER.get(this);
}
+ protected void registerDebug() {
+ if (debugStatus == null) {
+ debugStatus = new DebugState(this.toString());
+ ObjectCleaner.register(this, debugStatus);
+ }
+ }
Review Comment:
As this is basically a separated-out part of the constructor, to be used by
specific subclasses, it should probably go next to it, and maybe note that.
--
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]
