Fix the connection and manager lifecycle (#126, #94, #95, #98)

README.md

@@ -20,7 +20,7 @@ The BMC must have IPMI over LAN enabled and an account with the User privilege:
 
 ## Upgrading
 
-Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`.
+Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`. `IpmiConnector.closeConnection()` releases the connection, whose handle then throws `IllegalStateException`; the keep-alive is actually sent with the default configuration, as a `Connection.KeepAlive` request whose reply and timeout are not reported to the listeners; and `Constants.TIMEOUT`, which nothing reads, is deprecated.
 
 ## Build instructions
 

src/main/java/org/metricshub/ipmi/core/common/Constants.java

@@ -32,6 +32,12 @@ public final class Constants {
 	 */
 	public static final int IPMI_PORT = 0x26F;
 
+	/**
+	 * Unused by the library.
+	 *
+	 * @deprecated nothing reads it; it will be removed in the next major version.
+	 */
+	@Deprecated
 	public static final int TIMEOUT = 500;
 
 	private Constants() {}

src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java

@@ -26,38 +26,50 @@
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 public final class PropertiesManager {
 
+	private static final Object INSTANCE_LOCK = new Object();
 	private static PropertiesManager instance;
 
 	private Map<String, String> properties;
 
 	private Logger logger = LoggerFactory.getLogger(PropertiesManager.class);
 
 	private PropertiesManager() {
-		properties = new HashMap<String, String>();
+		properties = new ConcurrentHashMap<String, String>();
 
 		loadProperties("/connection.properties");
 		loadProperties("/vxipmi.properties");
 	}
 
 	@SuppressFBWarnings(value = "MS_EXPOSE_REP", justification = "Singleton: handing out the shared instance is the point")
 	public static PropertiesManager getInstance() {
-		if (instance == null) {
-			instance = new PropertiesManager();
+		synchronized (INSTANCE_LOCK) {
+			if (instance == null) {
+				instance = new PropertiesManager();
+			}
+			return instance;
 		}
-		return instance;
 	}
 
-	private void loadProperties(String name) {
+	/**
+	 * Adds the properties of a classpath resource; a missing resource is logged and skipped.
+	 *
+	 * @param name the resource name, such as {@code /connection.properties}
+	 */
+	void loadProperties(String name) {
 		try (InputStream stream = getClass().getResourceAsStream(name)) {
+			if (stream == null) {
+				logger.error("Properties resource {} not found", name);
+				return;
+			}
 			Properties props = new Properties();
 			props.load(stream);
 
@@ -71,7 +83,7 @@ private void loadProperties(String name) {
 	}
 
 	public String getProperty(String key) {
-		logger.info("Getting " + key + ": " + properties.get(key));
+		logger.debug("Getting {}: {}", key, properties.get(key));
 		return properties.get(key);
 	}
 

src/main/java/org/metricshub/ipmi/core/connection/Connection.java

@@ -26,6 +26,7 @@
 import org.metricshub.ipmi.core.coding.commands.IpmiVersion;
 import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel;
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
+import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilities;
 import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilitiesResponseData;
 import org.metricshub.ipmi.core.coding.commands.session.GetChannelCipherSuitesResponseData;
 import org.metricshub.ipmi.core.coding.commands.session.OpenSessionResponseData;
@@ -61,6 +62,7 @@
 import org.metricshub.ipmi.core.sm.states.Authcap;
 import org.metricshub.ipmi.core.sm.states.Ciphers;
 import org.metricshub.ipmi.core.sm.states.SessionValid;
+import org.metricshub.ipmi.core.sm.states.State;
 import org.metricshub.ipmi.core.sm.states.Uninitialized;
 import org.metricshub.ipmi.core.transport.Messenger;
 
@@ -666,31 +668,35 @@ public void notify(StateMachineAction action) {
 	}
 
 	/**
-	 * {@link TimerTask} runner - periodically sends no-op messages to keep the
-	 * session up
+	 * The keep-alive request: queued like any request, so that its tag stays reserved until its reply arrives or it
+	 * times out, but owned by nobody: its reply is discarded and its timeout is not reported to the listeners.
+	 */
+	public static final class KeepAlive extends GetChannelAuthenticationCapabilities {
+		/**
+		 * Creates the keep-alive request of a session.
+		 *
+		 * @param cipherSuite the {@link CipherSuite} of the session
+		 */
+		public KeepAlive(CipherSuite cipherSuite) {
+			super(IpmiVersion.V20, IpmiVersion.V20, cipherSuite, PrivilegeLevel.Callback, TypeConverter.intToByte(0xe));
+		}
+	}
+
+	/**
+	 * {@link TimerTask} runner - periodically sends a no-op message to keep the session up. The message is a
+	 * {@link KeepAlive}: its reply is discarded, while a reply to the same command sent by the application is
+	 * delivered to the application. When the message queue is full, the keep-alive of this period is skipped.
 	 */
 	@Override
 	public void run() {
-		int result = -1;
-		while (!Thread.currentThread().isInterrupted()
-				&& result <= 0
-				&& stateMachine.getCurrent() instanceof SessionValid) {
-			try {
-
-				result = sendMessage(
-						new org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilities(
-								IpmiVersion.V20,
-								IpmiVersion.V20,
-								((SessionValid) stateMachine.getCurrent()).getCipherSuite(),
-								PrivilegeLevel.Callback,
-								TypeConverter.intToByte(0xe)),
-						false);
-
-				Thread.sleep(1000);
-
-			} catch (Exception e) {
-				LOGGER.error(e.getMessage(), e);
-			}
+		State current = stateMachine.getCurrent();
+		if (!stateMachine.isActive() || !(current instanceof SessionValid)) {
+			return;
+		}
+		try {
+			sendMessage(new KeepAlive(((SessionValid) current).getCipherSuite()), false);
+		} catch (Exception e) {
+			LOGGER.error("Keep-alive failed: " + e.getMessage(), e);
 		}
 	}