From 5730fcacfa186b0fe4257d75e4c1a1b2d3c403fa Mon Sep 17 00:00:00 2001 From: Clebert Suconic Date: Thu, 2 Dec 2021 17:39:53 -0500 Subject: [PATCH] ARTEMIS-3599 Removing finalization calls --- .../factory/ConnectionFactoryProvider.java | 3 +- .../api/core/client/ServerLocator.java | 10 -- .../client/impl/ClientSessionFactoryImpl.java | 18 -- .../impl/ClientSessionFactoryInternal.java | 2 - .../core/client/impl/ServerLocatorImpl.java | 31 +--- .../remoting/impl/netty/NettyConnector.java | 6 - .../jms/client/ActiveMQConnection.java | 11 -- .../jms/client/ActiveMQConnectionFactory.java | 30 +--- .../cluster/impl/ClusterConnectionImpl.java | 1 - .../recovery/ActiveMQXAResourceWrapper.java | 1 - .../connection/CloseConnectionOnGCTest.java | 159 ------------------ 11 files changed, 4 insertions(+), 268 deletions(-) delete mode 100644 tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/CloseConnectionOnGCTest.java diff --git a/artemis-cdi-client/src/main/java/org/apache/artemis/client/cdi/factory/ConnectionFactoryProvider.java b/artemis-cdi-client/src/main/java/org/apache/artemis/client/cdi/factory/ConnectionFactoryProvider.java index de53dc07c0..94f1152555 100644 --- a/artemis-cdi-client/src/main/java/org/apache/artemis/client/cdi/factory/ConnectionFactoryProvider.java +++ b/artemis-cdi-client/src/main/java/org/apache/artemis/client/cdi/factory/ConnectionFactoryProvider.java @@ -95,7 +95,6 @@ public class ConnectionFactoryProvider { activeMQConnectionFactory.setUser(configuration.getUsername()); activeMQConnectionFactory.setPassword(configuration.getPassword()); } - // The CF will probably be GCed since it was injected, so we disable the finalize check - return activeMQConnectionFactory.disableFinalizeChecks(); + return activeMQConnectionFactory; } } diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/client/ServerLocator.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/client/ServerLocator.java index 937cadf9ff..49e6c29396 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/client/ServerLocator.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/client/ServerLocator.java @@ -47,16 +47,6 @@ public interface ServerLocator extends AutoCloseable { */ boolean isClosed(); - /** - * This method will disable any checks when a GarbageCollection happens - * leaving connections open. The JMS Layer will make specific usage of this - * method, since the ConnectionFactory.finalize should release this. - *

- * Warning: You may leave resources unattended if you call this method and - * don't take care of cleaning the resources yourself. - */ - void disableFinalizeCheck(); - /** * Creates a ClientSessionFactory using whatever load balancing policy is in force * diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java index 48e70ff1e3..be4a9cfb93 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java @@ -90,8 +90,6 @@ public class ClientSessionFactoryImpl implements ClientSessionFactoryInternal, C private ConnectorFactory connectorFactory; - private transient boolean finalizeCheck = true; - private final long callTimeout; private final long callFailoverTimeout; @@ -240,11 +238,6 @@ public class ClientSessionFactoryImpl implements ClientSessionFactoryInternal, C } } - @Override - public void disableFinalizeCheck() { - finalizeCheck = false; - } - @Override public Lock lockFailover() { newFailoverLock.lock(); @@ -1037,17 +1030,6 @@ public class ClientSessionFactoryImpl implements ClientSessionFactoryInternal, C } } - @Override - protected void finalize() throws Throwable { - if (!closed && finalizeCheck) { - ActiveMQClientLogger.LOGGER.factoryLeftOpen(createTrace, System.identityHashCode(this)); - - close(); - } - - super.finalize(); - } - protected ConnectorFactory instantiateConnectorFactory(final String connectorFactoryClassName) { // Will set the instance here to avoid races where cachedFactory is set to null diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryInternal.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryInternal.java index 2c57c5a5a1..45cab8a3dc 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryInternal.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryInternal.java @@ -35,8 +35,6 @@ public interface ClientSessionFactoryInternal extends ClientSessionFactory { boolean waitForTopology(long timeout, TimeUnit unit); - void disableFinalizeCheck(); - String getLiveNodeId(); // for testing diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java index 62fbef1fba..17707780fd 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java @@ -99,7 +99,8 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery private final boolean ha; - private boolean finalizeCheck = true; + // this is not used... I'm only keeping it here because of Serialization compatibiity and Wildfly usage on JNDI. + private boolean finalizeCheck; private boolean clusterConnection; @@ -410,7 +411,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery private ServerLocatorImpl(ServerLocatorImpl locator) { ha = locator.ha; - finalizeCheck = locator.finalizeCheck; clusterConnection = locator.clusterConnection; initialConnectors = locator.initialConnectors; discoveryGroupConfiguration = locator.discoveryGroupConfiguration; @@ -523,11 +523,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery return this; } - @Override - public void disableFinalizeCheck() { - finalizeCheck = false; - } - @Override public ClientSessionFactoryInternal connect() throws ActiveMQException { return connect(false); @@ -1310,15 +1305,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery return this; } - @Override - protected void finalize() throws Throwable { - if (finalizeCheck) { - close(); - } - - super.finalize(); - } - @Override public void cleanup() { doClose(false); @@ -1774,8 +1760,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery for (TransportConfiguration initialConnector : initialConnectors) { ClientSessionFactoryInternal factory = new ClientSessionFactoryImpl(ServerLocatorImpl.this, initialConnector, config, config.reconnectAttempts, threadPool, scheduledThreadPool, incomingInterceptors, outgoingInterceptors); - factory.disableFinalizeCheck(); - connectors.add(new Connector(initialConnector, factory)); } } @@ -1789,17 +1773,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery } } - @Override - protected void finalize() throws Throwable { - if (!isClosed() && finalizeCheck) { - ActiveMQClientLogger.LOGGER.serverLocatorNotClosed(traceException, System.identityHashCode(this)); - - close(); - } - - super.finalize(); - } - private final class Connector { private final TransportConfiguration initialConnector; diff --git a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnector.java b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnector.java index b1d183a6fe..05ed85d2db 100644 --- a/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnector.java +++ b/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnector.java @@ -1299,12 +1299,6 @@ public class NettyConnector extends AbstractConnector { return false; } - @Override - public void finalize() throws Throwable { - close(); - super.finalize(); - } - //for test purpose only public Bootstrap getBootStrap() { return bootstrap; diff --git a/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java b/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java index f2e468f355..c7eb4c2edc 100644 --- a/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java +++ b/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java @@ -557,17 +557,6 @@ public class ActiveMQConnection extends ActiveMQConnectionForContextImpl impleme return initialSession; } - @Override - protected final void finalize() throws Throwable { - if (!closed) { - if (this.factoryReference.isFinalizeChecks()) { - ActiveMQJMSClientLogger.LOGGER.connectionLeftOpen(creationStack); - } - - close(); - } - } - protected boolean isXA() { return false; } diff --git a/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java b/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java index 27bfa7d2e9..915fee0848 100644 --- a/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java +++ b/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java @@ -94,6 +94,7 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio private boolean cacheDestinations; + // keeping this field for serialization compatibility only. do not use it private boolean finalizeChecks; private boolean ignoreJTA; @@ -165,16 +166,6 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio } } - public ActiveMQConnectionFactory disableFinalizeChecks() { - this.finalizeChecks = false; - return this; - } - - public boolean isFinalizeChecks() { - return finalizeChecks; - } - - @Override public String getDeserializationBlackList() { return deserializationBlackList; @@ -265,8 +256,6 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio public ActiveMQConnectionFactory(final ServerLocator serverLocator) { this.serverLocator = serverLocator; - - serverLocator.disableFinalizeCheck(); } public ActiveMQConnectionFactory(final boolean ha, final DiscoveryGroupConfiguration groupConfiguration) { @@ -275,8 +264,6 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio } else { serverLocator = ActiveMQClient.createServerLocatorWithoutHA(groupConfiguration); } - - serverLocator.disableFinalizeCheck(); } public ActiveMQConnectionFactory(final boolean ha, final TransportConfiguration... initialConnectors) { @@ -285,8 +272,6 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio } else { serverLocator = ActiveMQClient.createServerLocatorWithoutHA(initialConnectors); } - - serverLocator.disableFinalizeCheck(); } @Override @@ -950,19 +935,6 @@ public class ActiveMQConnectionFactory extends JNDIStorable implements Connectio } } - @Override - protected void finalize() throws Throwable { - try { - if (serverLocator != null) { - serverLocator.close(); - } - } catch (Exception e) { - e.printStackTrace(); - //not much we can do here - } - super.finalize(); - } - // this may need to be set by classes which extend this class protected void makeReadOnly() { this.readOnly = true; diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java index 64939ad514..902071652e 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionImpl.java @@ -897,7 +897,6 @@ public final class ClusterConnectionImpl implements ClusterConnection, AfterConn targetLocator.setRetryInterval(retryInterval); } - targetLocator.disableFinalizeCheck(); targetLocator.addIncomingInterceptor(new IncomingInterceptorLookingForExceptionMessage(manager, executorFactory.getExecutor())); MessageFlowRecordImpl record = new MessageFlowRecordImpl(targetLocator, eventUID, targetNodeID, connector, queueName, queue); diff --git a/artemis-service-extensions/src/main/java/org/apache/activemq/artemis/service/extensions/xa/recovery/ActiveMQXAResourceWrapper.java b/artemis-service-extensions/src/main/java/org/apache/activemq/artemis/service/extensions/xa/recovery/ActiveMQXAResourceWrapper.java index 30fa6647a6..7129e08732 100644 --- a/artemis-service-extensions/src/main/java/org/apache/activemq/artemis/service/extensions/xa/recovery/ActiveMQXAResourceWrapper.java +++ b/artemis-service-extensions/src/main/java/org/apache/activemq/artemis/service/extensions/xa/recovery/ActiveMQXAResourceWrapper.java @@ -309,7 +309,6 @@ public class ActiveMQXAResourceWrapper implements XAResource, SessionFailureList if (xaRecoveryConfig.getLocatorConfig() != null) { serverLocator.setLocatorConfig(xaRecoveryConfig.getLocatorConfig()); } - serverLocator.disableFinalizeCheck(); serverLocator.setProtocolManagerFactory(xaRecoveryConfig.getClientProtocolManager()); csf = serverLocator.createSessionFactory(); if (xaRecoveryConfig.getUsername() == null) { diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/CloseConnectionOnGCTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/CloseConnectionOnGCTest.java deleted file mode 100644 index 427fbfbfaa..0000000000 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/connection/CloseConnectionOnGCTest.java +++ /dev/null @@ -1,159 +0,0 @@ -/* - * 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.integration.jms.connection; - -import javax.jms.Connection; -import javax.jms.Session; -import java.lang.ref.WeakReference; -import java.util.Iterator; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; - -import org.apache.activemq.artemis.api.core.TransportConfiguration; -import org.apache.activemq.artemis.api.jms.ActiveMQJMSClient; -import org.apache.activemq.artemis.api.jms.JMSFactoryType; -import org.apache.activemq.artemis.core.remoting.CloseListener; -import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory; -import org.apache.activemq.artemis.spi.core.protocol.RemotingConnection; -import org.apache.activemq.artemis.tests.util.ActiveMQTestBase; -import org.apache.activemq.artemis.tests.util.JMSTestBase; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; - -/** - * A CloseConnectionOnGCTest - */ -public class CloseConnectionOnGCTest extends JMSTestBase { - - private ActiveMQConnectionFactory cf; - - @Override - @Before - public void setUp() throws Exception { - super.setUp(); - - cf = ActiveMQJMSClient.createConnectionFactoryWithoutHA(JMSFactoryType.CF, new TransportConfiguration(INVM_CONNECTOR_FACTORY)); - cf.setBlockOnDurableSend(true); - cf.setPreAcknowledge(true); - } - - @Test - public void testCloseOneConnectionOnGC() throws Exception { - // Debug - don't remove this until intermittent failure with this test is fixed - int initialConns = server.getRemotingService().getConnections().size(); - - Assert.assertEquals(0, initialConns); - - Connection conn = cf.createConnection(); - - WeakReference wr = new WeakReference<>(conn); - - Assert.assertEquals(1, server.getRemotingService().getConnections().size()); - final CountDownLatch latch = new CountDownLatch(1); - Iterator connectionIterator = server.getRemotingService().getConnections().iterator(); - connectionIterator.next().addCloseListener(new CloseListener() { - @Override - public void connectionClosed() { - latch.countDown(); - } - }); - - conn = null; - - ActiveMQTestBase.checkWeakReferences(wr); - - latch.await(5000, TimeUnit.MILLISECONDS); - Assert.assertEquals(0, server.getRemotingService().getConnections().size()); - } - - @Test - public void testCloseSeveralConnectionOnGC() throws Exception { - Connection conn1 = cf.createConnection(); - Connection conn2 = cf.createConnection(); - Connection conn3 = cf.createConnection(); - - WeakReference wr1 = new WeakReference<>(conn1); - WeakReference wr2 = new WeakReference<>(conn2); - WeakReference wr3 = new WeakReference<>(conn3); - - Assert.assertEquals(3, server.getRemotingService().getConnections().size()); - - final CountDownLatch latch = new CountDownLatch(3); - Iterator connectionIterator = server.getRemotingService().getConnections().iterator(); - while (connectionIterator.hasNext()) { - RemotingConnection remotingConnection = connectionIterator.next(); - remotingConnection.addCloseListener(new CloseListener() { - @Override - public void connectionClosed() { - latch.countDown(); - } - }); - } - - conn1 = null; - conn2 = null; - conn3 = null; - - ActiveMQTestBase.checkWeakReferences(wr1, wr2, wr3); - - latch.await(5000, TimeUnit.MILLISECONDS); - - Assert.assertEquals(0, server.getRemotingService().getConnections().size()); - } - - @Test - public void testCloseSeveralConnectionsWithSessionsOnGC() throws Exception { - Connection conn1 = cf.createConnection(); - Connection conn2 = cf.createConnection(); - Connection conn3 = cf.createConnection(); - - WeakReference wr1 = new WeakReference<>(conn1); - WeakReference wr2 = new WeakReference<>(conn2); - WeakReference wr3 = new WeakReference<>(conn3); - - Session sess1 = conn1.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess2 = conn1.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess3 = conn2.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess4 = conn2.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess5 = conn3.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess6 = conn3.createSession(false, Session.AUTO_ACKNOWLEDGE); - Session sess7 = conn3.createSession(false, Session.AUTO_ACKNOWLEDGE); - final CountDownLatch latch = new CountDownLatch(3); - Iterator connectionIterator = server.getRemotingService().getConnections().iterator(); - while (connectionIterator.hasNext()) { - RemotingConnection remotingConnection = connectionIterator.next(); - remotingConnection.addCloseListener(new CloseListener() { - @Override - public void connectionClosed() { - latch.countDown(); - } - }); - } - sess1 = sess2 = sess3 = sess4 = sess5 = sess6 = sess7 = null; - - conn1 = null; - conn2 = null; - conn3 = null; - - ActiveMQTestBase.checkWeakReferences(wr1, wr2, wr3); - - latch.await(5000, TimeUnit.MILLISECONDS); - - Assert.assertEquals(0, server.getRemotingService().getConnections().size()); - } -}