modules/core/plugin-container-itest/src/test/java/org/rhq/core/pc/avail/AvailTest.java | 20 modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/CmdlineClient.java | 7 modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/JBossRemotingRemoteCommunicator.java | 322 ++-------- 3 files changed, 103 insertions(+), 246 deletions(-)
New commits: commit 7f08f14c4c8a025b169e32e0378335cf9a243c36 Author: Jay Shaughnessy jshaughn@redhat.com Date: Thu Jan 30 11:55:47 2014 -0500
Fix iPC -tests, allow negative resource ids; now possible for tests based on FakeServerInventpory.
diff --git a/modules/core/plugin-container-itest/src/test/java/org/rhq/core/pc/avail/AvailTest.java b/modules/core/plugin-container-itest/src/test/java/org/rhq/core/pc/avail/AvailTest.java index 5c9a5ca..85e1ec2 100644 --- a/modules/core/plugin-container-itest/src/test/java/org/rhq/core/pc/avail/AvailTest.java +++ b/modules/core/plugin-container-itest/src/test/java/org/rhq/core/pc/avail/AvailTest.java @@ -195,7 +195,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.isChangesOnlyReport(), false, "First report should have been a full report"); List<Datum> availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource ID should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } AvailabilityExecutor.Scan scan = executor.getMostRecentScanHistory(); @@ -238,7 +238,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.isChangesOnlyReport(), false, "First report should have been a full report"); List<Datum> availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } AvailabilityExecutor.Scan scan = executor.getMostRecentScanHistory(); @@ -297,7 +297,7 @@ public class AvailTest extends Arquillian { List<Datum> availData = report.getResourceAvailability(); int numUp = 0; for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; if (datum.getAvailabilityType() == AvailabilityType.UP) { ++numUp; } @@ -342,7 +342,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.getResourceAvailability().size(), 14, "should report half the resources"); availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } scan = executor.getMostRecentScanHistory(); @@ -362,7 +362,7 @@ public class AvailTest extends Arquillian { List<Datum> availData = report.getResourceAvailability(); int numUp = 0; for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be > zero since it should be committed"; if (datum.getAvailabilityType() == AvailabilityType.UP) { ++numUp; } @@ -414,7 +414,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.getResourceAvailability().size(), 14, "should report half the resources"); availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.DOWN, "should be DOWN"); } scan = executor.getMostRecentScanHistory(); @@ -433,7 +433,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.isChangesOnlyReport(), false, "First report should have been a full report"); List<Datum> availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } AvailabilityExecutor.Scan scan = executor.getMostRecentScanHistory(); @@ -474,7 +474,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.getResourceAvailability().size(), 0, "no changes, everything was already up"); availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } scan = executor.getMostRecentScanHistory(); @@ -499,7 +499,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.isChangesOnlyReport(), false, "First report should have been a full report"); List<Datum> availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } AvailabilityExecutor.Scan scan = executor.getMostRecentScanHistory(); @@ -525,7 +525,7 @@ public class AvailTest extends Arquillian { Assert.assertEquals(report.getResourceAvailability().size(), 0, "no changes, everything was already up"); availData = report.getResourceAvailability(); for (Datum datum : availData) { - assert datum.getResourceId() > 0 : "resource IDs should be > zero since it should be committed"; + assert datum.getResourceId() != 0 : "resource IDs should be != zero since it should be committed"; Assert.assertEquals(datum.getAvailabilityType(), AvailabilityType.UP, "should be UP at the start"); } scan = executor.getMostRecentScanHistory();
commit 37263f7ece17f28541702666009fe057a28452c1 Author: Jay Shaughnessy jshaughn@redhat.com Date: Thu Jan 30 11:11:46 2014 -0500
BZ 1052390 - Clean up remoting wrapper to avoid race conditions if possible
Some unused or rarely constructors and methods were dropped.
The biggest change is in the client caching. The cache code is guaranteed to call disconnect when a client is 'thrown away'. There is still a possibility disconnect can happen in the middle of an invoke.
Original Author: Elias Ross elias_ross@apple.com Signed-off-by: Jay Shaughnessy jshaughn@redhat.com Applying this patch as-is, I see no issues with it and it cleans some things up. Relevant tests are passing.
diff --git a/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/CmdlineClient.java b/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/CmdlineClient.java index 5e42cc2..a2186d0 100644 --- a/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/CmdlineClient.java +++ b/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/CmdlineClient.java @@ -20,6 +20,7 @@ package org.rhq.enterprise.communications.command.client;
import gnu.getopt.Getopt; import gnu.getopt.LongOpt; + import java.net.MalformedURLException; import java.util.ArrayList; import java.util.HashMap; @@ -27,7 +28,10 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.StringTokenizer; + import mazz.i18n.Logger; + +import org.jboss.remoting.InvokerLocator; import org.rhq.enterprise.communications.command.Command; import org.rhq.enterprise.communications.command.CommandResponse; import org.rhq.enterprise.communications.command.CommandType; @@ -212,7 +216,8 @@ public class CmdlineClient { throw new MalformedURLException(LOG.getMsgString(CommI18NResourceKeys.CMDLINE_CLIENT_NULL_URI)); }
- JBossRemotingRemoteCommunicator communicator = new JBossRemotingRemoteCommunicator(m_locatorUri, m_subsystem); + InvokerLocator invokerLocator = new InvokerLocator(m_locatorUri); + JBossRemotingRemoteCommunicator communicator = new JBossRemotingRemoteCommunicator(invokerLocator, m_subsystem, null); commandClient.setRemoteCommunicator(communicator);
// tell the concrete command client instance to invoke the command on the remote server diff --git a/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/JBossRemotingRemoteCommunicator.java b/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/JBossRemotingRemoteCommunicator.java index 0acc1c7..9bcf181 100644 --- a/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/JBossRemotingRemoteCommunicator.java +++ b/modules/enterprise/comm/src/main/java/org/rhq/enterprise/communications/command/client/JBossRemotingRemoteCommunicator.java @@ -22,6 +22,7 @@ import java.net.MalformedURLException; import java.util.HashMap; import java.util.Map; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;
@@ -30,7 +31,6 @@ import mazz.i18n.Logger; import org.jboss.remoting.Client; import org.jboss.remoting.InvokerLocator; import org.jboss.remoting.ServerInvoker; - import org.rhq.core.util.exception.ThrowableUtil; import org.rhq.enterprise.communications.command.Command; import org.rhq.enterprise.communications.command.CommandResponse; @@ -47,12 +47,11 @@ import org.rhq.enterprise.communications.util.NotPermittedException; * where the command is to be invoked.</p> * * <p>Under the covers, a {@link org.jboss.remoting.Client remoting client} is created and maintained by this object. - * The users of this object may manually {@link #connect() connect} and {@link #disconnect() disconnect} that remoting - * client. Typically, there will not be a need to connect since it will be done automatically when appropriate; however, - * it is good practice to tell this object to disconnect its remoting client when this object is no longer needed to + * There is no need to call {@link #connect()} since it will be done automatically when appropriate; however, + * it is good practice to tell this object to {@link #disconnect()} its client when no longer necessary to * issue commands to the remote server.</p> * - * <p>All subclasses should include a no-arg constructor so they can be built dynamically by the cmdline client.</p> + * <p>All subclasses should include a no-arg constructor so they can be built dynamically by the command line client.</p> * * @author John Mazzitelli */ @@ -67,41 +66,40 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { /** * the JBoss/Remoting locator that this client will use to remotely connect to the command server */ - private InvokerLocator m_invokerLocator; + private volatile InvokerLocator m_invokerLocator;
/** * The subsystem to target when invoking commands. The subsystem is defined by the JBoss/Remoting API - it specifies * the actual invoker handler to target. The Command framework uses the subsystem to organize command processors * into different domains. */ - private String m_subsystem; + private final String m_subsystem;
/** * The configuration to send to the client - used to configure things like the SSL setup. */ - private Map<String, String> m_clientConfiguration; + private final Map<String, String> m_clientConfiguration;
/** * the actual JBoss/Remoting client object that will be used to transport the commands to the server */ - private Client m_remotingClient; + private volatile AtomicReference<Client> m_client = new AtomicReference<Client>();
/** - * Optionally-defined callback that will be called when a failure is detected when sending a message. + * Optionally-defined callback that will be called when a failure is detected when sending a message. */ - private FailureCallback m_failureCallback; + private volatile FailureCallback m_failureCallback;
/** - * Optionally-defined callback that will be called when this communicator sends its first command - * after it has been {@link #connect() connected}. + * Optionally-defined callback that will be called when this communicator sends its first command. */ - private InitializeCallback m_initializeCallback; + private volatile InitializeCallback m_initializeCallback;
/** * When <code>true</code>, the initialize callback will need to be called prior * to sending any commands. Used in conjunection with its associated RW lock. */ - private boolean m_needToCallInitializeCallback; + private volatile boolean m_needToCallInitializeCallback;
/** * RW lock when needing to access its associated atomic boolean flag. @@ -109,25 +107,13 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { private final ReentrantReadWriteLock m_needToCallInitializeCallbackLock;
/** - * Number of minutes to wait while attempting to aquire a lock before attempting + * Number of minutes to wait while attempting to acquire a lock before attempting * to invoke the initialize callback. If this amount of minutes expires before the lock * is acquired, an error will occur and the initialize callback will have to be attempted later. */ private final long m_initializeCallbackLockAcquisitionTimeoutMins;
/** - * Constructor for {@link JBossRemotingRemoteCommunicator} that initializes the client with no invoker locator - * defined. It must later be specified through {@link #setInvokerLocator(InvokerLocator)} before any client commands - * can be issued. In addition, the {@link #getSubsystem()} will be set to the {@link #DEFAULT_SUBSYSTEM}. - * - * <p>Note that all subclasses are strongly urged to include this no-arg constructor so it can plug into the cmdline - * client seamlessly.</p> - */ - public JBossRemotingRemoteCommunicator() { - this((InvokerLocator) null, DEFAULT_SUBSYSTEM); - } - - /** * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to indicate the * {@link InvokerLocator invoker locator} to use by specifying the locator's URI. The subsystem will be set to the * {@link #DEFAULT_SUBSYSTEM}. @@ -137,7 +123,7 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { * @throws MalformedURLException if failed to create the locator (see {@link InvokerLocator#InvokerLocator(String)}) */ public JBossRemotingRemoteCommunicator(String locatorUri) throws MalformedURLException { - this(new InvokerLocator(locatorUri), DEFAULT_SUBSYSTEM); + this(new InvokerLocator(locatorUri), DEFAULT_SUBSYSTEM, null); }
/** @@ -160,45 +146,6 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to specify the * {@link InvokerLocator invoker locator} to use. <code>locator</code> may be <code>null</code>, in which case, it * must later be specified through {@link #setInvokerLocator(InvokerLocator)} before any client commands can be - * issued. The subsystem will be set to the {@link #DEFAULT_SUBSYSTEM}. - * - * @param locator the locator to use (may be <code>null</code>) - */ - public JBossRemotingRemoteCommunicator(InvokerLocator locator) { - this(locator, DEFAULT_SUBSYSTEM); - } - - /** - * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to specify the - * {@link InvokerLocator invoker locator} to use. <code>locator</code> may be <code>null</code>, in which case, it - * must later be specified through {@link #setInvokerLocator(InvokerLocator)} before any client commands can be - * issued.The subsystem will be set to the {@link #DEFAULT_SUBSYSTEM}. The given <code>Map</code> should contain - * <code>Client</code> configuration attributes. - * - * @param locator the locator to use (may be <code>null</code>) - * @param client_config the client configuration (may be <code>null</code> or empty) - */ - public JBossRemotingRemoteCommunicator(InvokerLocator locator, Map<String, String> client_config) { - this(locator, DEFAULT_SUBSYSTEM, client_config); - } - - /** - * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to specify the - * {@link InvokerLocator invoker locator} to use. <code>locator</code> may be <code>null</code>, in which case, it - * must later be specified through {@link #setInvokerLocator(InvokerLocator)} before any client commands can be - * issued. - * - * @param locator the locator to use (may be <code>null</code>) - * @param subsystem the subsystem (or command domain) in which commands will be invoked (may be <code>null</code>) - */ - public JBossRemotingRemoteCommunicator(InvokerLocator locator, String subsystem) { - this(locator, subsystem, null); - } - - /** - * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to specify the - * {@link InvokerLocator invoker locator} to use. <code>locator</code> may be <code>null</code>, in which case, it - * must later be specified through {@link #setInvokerLocator(InvokerLocator)} before any client commands can be * issued. * * @param locator the locator to use (may be <code>null</code>) @@ -222,7 +169,7 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { try { String minsStr = System.getProperty("rhq.communications.initial-callback-lock-wait-mins", "60"); mins = Long.parseLong(minsStr); - } catch (Throwable t) { + } catch (Exception e) { mins = 60L; } m_initializeCallbackLockAcquisitionTimeoutMins = mins; @@ -231,36 +178,6 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { }
/** - * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to indicate the - * {@link InvokerLocator invoker locator} to use by specifying the locator's URI. - * - * @param locatorUri the locator's URI (must not be <code>null</code>) - * @param subsystem the subsystem (or command domain) in which commands will be invoked (may be <code>null</code>) - * - * @throws MalformedURLException if failed to create the locator (see {@link InvokerLocator#InvokerLocator(String)}) - */ - public JBossRemotingRemoteCommunicator(String locatorUri, String subsystem) throws MalformedURLException { - this(new InvokerLocator(locatorUri), subsystem); - } - - /** - * Constructor for {@link JBossRemotingRemoteCommunicator} that allows you to indicate the - * {@link InvokerLocator invoker locator} to use by specifying the locator's URI. The given <code>Map</code> should - * contain <code>Client</code> configuration attributes. - * - * @param locatorUri the locator's URI (must not be <code>null</code>) - * @param subsystem the subsystem (or command domain) in which commands will be invoked (may be <code> - * null</code>) - * @param client_config the client configuration (may be <code>null</code> or empty) - * - * @throws MalformedURLException if failed to create the locator (see {@link InvokerLocator#InvokerLocator(String)}) - */ - public JBossRemotingRemoteCommunicator(String locatorUri, String subsystem, Map<String, String> client_config) - throws MalformedURLException { - this(new InvokerLocator(locatorUri), subsystem, client_config); - } - - /** * Returns the invoker locator that is to be used to find the remote JBoss/Remoting server. If <code>null</code> is * returned, this communicator will not be able to issue commands. * @@ -281,81 +198,14 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { * null</code>) * * @throws MalformedURLException if failed to create the locator (see {@link InvokerLocator#InvokerLocator(String)}) - * - * @see #setInvokerLocator(InvokerLocator) - */ - public void setInvokerLocator(String locatorUri) throws MalformedURLException { - setInvokerLocator(new InvokerLocator(locatorUri)); - } - - /** - * Sets the invoker locator URI and creates a new locator that is to be used to find the remote JBoss/Remoting - * server for its subsequent command client invocations. Any existing remoting client is automatically disconnected. - * New remoting clients will also be configured with the new set of configuration properties - thus allowing you to - * configure the client to be able to handle the new locator. - * - * @param locatorUri the new invoker locator's URI to use for future command client invocations (must not be - * <code>null</code>) - * @param client_config the client configuration for any new remoting clients that are created (may be <code> - * null</code> or empty) - * - * @throws MalformedURLException if failed to create the locator (see {@link InvokerLocator#InvokerLocator(String)}) - * - * @see #setInvokerLocator(InvokerLocator) - */ - public void setInvokerLocator(String locatorUri, Map<String, String> client_config) throws MalformedURLException { - setInvokerLocator(new InvokerLocator(locatorUri), client_config); - } - - /** - * Sets the invoker locator that this communicator should use for its subsequent command client invocations. Any - * existing remoting client is automatically disconnected.The client configuration properties will, however, remain - * the same as before - so the new clients that are created will have the same configuration attributes. See - * {@link #setInvokerLocator(InvokerLocator, Map)} if you want to reconfigure the client with different properties - * that are more appropriate for the new locator. - * - * @param locator the new invoker locator to use for future command client invocations (must not be <code> - * null</code>) - * - * @throws IllegalArgumentException if locator is <code>null</code> - */ - public void setInvokerLocator(InvokerLocator locator) { - setInvokerLocator(locator, null); - } - - /** - * Sets the invoker locator that this communicator should use for its subsequent command client invocations. Any - * existing remoting client is automatically disconnected. New remoting clients will also be configured with the new - * set of configuration properties - thus allowing you to configure the client to be able to handle the new locator. - * - * @param locator the new invoker locator to use for future command client invocations (must not be <code> - * null</code>) - * @param client_config the client configuration for any new remoting clients that are created (may be <code> - * null</code> or empty) - * - * @throws IllegalArgumentException if locator is <code>null</code> */ - public void setInvokerLocator(InvokerLocator locator, Map<String, String> client_config) { - if (locator == null) { - throw new IllegalArgumentException("locator=null"); - } - - // since a new invoker locator is being specified, disconnect any old client that already exists - if (m_remotingClient != null) { - m_remotingClient.disconnect(); - m_remotingClient = null; - m_needToCallInitializeCallback = (getInitializeCallback() != null); // specifically do not synchrononize by using lock, just set it - } - + public void setRemoteEndpoint(String endpoint) throws Exception { + InvokerLocator locator = new InvokerLocator(endpoint); LOG.info(CommI18NResourceKeys.COMMUNICATOR_CHANGING_ENDPOINT, m_invokerLocator, locator); m_invokerLocator = locator;
- if (client_config != null) { - m_clientConfiguration.clear(); - m_clientConfiguration.putAll(client_config); - } - - return; + // since a new invoker locator is being specified, disconnect any old client that already exists + disconnect(); }
/** @@ -369,25 +219,6 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { return m_subsystem; }
- /** - * Sets the value of the subsystem that will be used to target command invocations. The subsystem is defined by the - * JBoss/Remoting API and can be used by the Command Framework to organize command processors into different - * domains. - * - * <p>If a remoting client already exists, its subsystem will be changed to the given subsystem.</p> - * - * @param subsystem the new value of subsystem (may be <code>null</code>) - */ - public void setSubsystem(String subsystem) { - m_subsystem = subsystem; - - if (m_remotingClient != null) { - m_remotingClient.setSubsystem(subsystem); - } - - return; - } - public FailureCallback getFailureCallback() { return m_failureCallback; } @@ -402,17 +233,13 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator {
public void setInitializeCallback(InitializeCallback callback) { m_initializeCallback = callback; - m_needToCallInitializeCallback = (callback != null); // specifically do not synchrononize by using lock, just set it + m_needToCallInitializeCallback = (callback != null); // specifically do not synchronize by using lock, just set it }
public String getRemoteEndpoint() { return (m_invokerLocator != null) ? m_invokerLocator.getLocatorURI() : "<null>"; }
- public void setRemoteEndpoint(String endpoint) throws Exception { - setInvokerLocator(endpoint); - } - /** * Returns the map of name/value pairs of client configuration settings used when creating the client. The returned * map is a copy - changing its contents has no effect on the clients that already have been or will be created by @@ -425,26 +252,31 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { return new HashMap<String, String>(m_clientConfiguration); }
+ /** + * Does nothing; send a request to connect. + */ public void connect() throws Exception { - if ((m_remotingClient != null) && !m_remotingClient.isConnected()) { - m_remotingClient.connect(); - m_needToCallInitializeCallback = (getInitializeCallback() != null); // specifically do not synchrononize by using lock, just set it + /* + * For the HTTP invoker, simply calling connect() doesn't do anything. It makes + * sense to at least send something to test connectivity. However, the code doesn't + * make use of this method. + try { + send(new EchoCommand()); + } catch (Exception e) { + throw e; + } catch (Throwable t) { + throw new Error(t); } - - return; + */ }
public void disconnect() { - if (m_remotingClient != null) { - m_remotingClient.disconnect(); - m_needToCallInitializeCallback = (getInitializeCallback() != null); // specifically do not synchrononize by using lock, just set it - } - - return; + cacheClient(null); }
public boolean isConnected() { - return (m_remotingClient != null) && m_remotingClient.isConnected(); + Client client = m_client.get(); + return (client != null) && client.isConnected(); }
public CommandResponse sendWithoutCallbacks(Command command) throws Throwable { @@ -455,7 +287,7 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { retry = false; ret_response = rawSend(command); Throwable exception = ret_response.getException(); - if ((exception != null) && (exception instanceof NotPermittedException)) { + if (exception instanceof NotPermittedException) { long pause = ((NotPermittedException) exception).getSleepBeforeRetry(); LOG.debug(CommI18NResourceKeys.COMMAND_NOT_PERMITTED, command, pause); retry = true; @@ -498,11 +330,11 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator {
/** * The code that sends the command via the remote client. - * + * * @param command the command to send - * + * * @return the command response - * + * * @throws Throwable if a low-level, unhandled exception occurred */ private CommandResponse rawSend(Command command) throws Throwable { @@ -511,13 +343,13 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { try { try { OutgoingCommandTrace.start(command); - ret_response = getRemotingClient().invoke(command, null); + ret_response = invoke(command); OutgoingCommandTrace.finish(command, ret_response); } catch (ServerInvoker.InvalidStateException serverDown) { // under rare condition, a bug in remoting 2.2 causes this when the server restarted // try it one more time, this will get a new server thread on the server side (JBREM-745) // once JBREM-745 is fixed, we can probably get rid of this catch block - ret_response = getRemotingClient().invoke(command, null); + ret_response = invoke(command); OutgoingCommandTrace.finish(command, ret_response); } } catch (Throwable t) { @@ -548,12 +380,12 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { * call it. The initialize callback has the responsibility to handle calling * {@link #sendWithoutInitializeCallback(Command)} if it wants to send its own commands to the server * but wants failover to happen when appropriate for those commands. - * + * * If there is an initialize callback set, this method will block all callers until * the callback has been invoked. * * @param command the command that it going to be sent after the callback is invoked - * + * * @return if the initialize callback had an error, this response will be non-<code>null</code> and * will indicate that the sending of <code>command</code> should be aborted. */ @@ -600,14 +432,14 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { /** * This will invoke the failure callback when necessary. It is necessary to call the callback * when the throwable is not <code>null</code> or the command response has a non-<code>null</code> exception. - * + * * This method will force a retry by returning <code>true</code>. If <code>false</code> is returned, * the request need not be retried. - * + * * @param command the command that was sent (or attempted to be sent) * @param response the response of the command (may be <code>null</code>) * @param throwable the exception that was thrown when the command was sent (may be <code>null</code>) - * + * * @return <code>true</code> if the command should be retried, <code>false</code> otherwise */ private boolean invokeFailureCallbackIfNeeded(Command command, CommandResponse response, Throwable throwable) { @@ -615,7 +447,7 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { FailureCallback callback = getFailureCallback(); // get a local reference to avoid this being changed underneath us boolean retry = false;
- // only do something if there is a callback defined + // only do something if there is a callback defined if (callback != null) { // only do something if the command resulted in an exception if (throwable != null || ((response != null) && (response.getException() != null))) { @@ -638,28 +470,48 @@ public class JBossRemotingRemoteCommunicator implements RemoteCommunicator { }
/** - * Returns the remoting client that is to be used to transport the command request to the server. If for any reason - * the client cannot be created, an exception is thrown. This will happen if the invoker locator has not been - * specified (see {@link #setInvokerLocator(InvokerLocator)}). - * - * <p>This method will cache the client and connect to the server automatically. Note that the client will be - * disconnected whenever the invoker is reset via {@link #setInvokerLocator(InvokerLocator)}. Therefore, callers - * should never cache the returned object themselves - always call this method to obtain a reference to the - * client.</p> - * - * @return the client to be used to transport the command request to the server + * Invokes JBoss Remoting using the given command. + * Attempts to cache the client if sending the message was successful. * - * @throws Exception if failed to create the client for whatever reason + * @return object as a result of this call */ - protected Client getRemotingClient() throws Exception { - if (m_remotingClient == null) { - m_remotingClient = new Client(getInvokerLocator(), getSubsystem(), m_clientConfiguration); + private Object invoke(Command command) throws Throwable { + InvokerLocator locator = m_invokerLocator; + if (locator == null) { + throw new IllegalStateException("m_invokerLocator is null"); }
- if (!m_remotingClient.isConnected()) { - m_remotingClient.connect(); + Client client = m_client.get(); + if (client != null && client.getInvoker() == null) { + client.disconnect(); + } + if (client == null || !client.isConnected()) { + client = new Client(locator, getSubsystem(), m_clientConfiguration); + client.connect(); + try { + return client.invoke(command); + } finally { + cacheClient(client); + } }
- return m_remotingClient; + // Note: Despite all the checks above, the client might have been + // disconnected before invoke is reached. Let's hope that doesn't happen. + + return client.invoke(command); + } + + /** + * Cache the client, disconnecting the old client. + * + * @param client optionally null; new client to cache + */ + private void cacheClient(Client client) { + Client old = m_client.getAndSet(client); + if (old != null) { + old.disconnect(); + m_needToCallInitializeCallback = (getInitializeCallback() != null); + } } + }