From ef8c6ed393ec1fc2ba5addcdf04b86a6f59c5ae7 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 16:45:52 +0200 Subject: [PATCH 1/8] Fix the connection and manager lifecycle ConnectionManager(int, long) assigns the ping period before initialize() resolves -1, so the default IpmiClient configuration gets the 30 s keep-alive of connection.properties instead of none (#126). The keep-alive task sends its Get Channel Authentication Capabilities one-way and returns: no retry loop that hot-spins when sendMessage throws, and StateMachine.stop() leaves the session state so a stopped connection reports no valid session (#94). As the keep-alive reply is no longer queued, IpmiMessageHandler delivers every reply, including the replies of that command sent by the application. The sessionless tag pool is final instead of being replaced by each new manager, every createConnection() overload returns the handle of the connection it created, closeConnection() releases the connection (getConnection() then throws IllegalStateException), getConnection( InetAddress, int) compares the addresses with equals(), and SessionManager.establishSession() closes the failed connection instead of tearing down the whole connector (#95). PropertiesManager guards its lazy initialization, skips a missing resource instead of failing with NullPointerException, keeps its map in a ConcurrentHashMap and logs lookups at DEBUG; the unused cleaningFrequency property and Constants.TIMEOUT are removed (#98). Co-Authored-By: Claude Fable 5.1 --- .../ipmi/core/common/Constants.java | 2 - .../ipmi/core/common/PropertiesManager.java | 26 +++-- .../ipmi/core/connection/Connection.java | 42 ++++---- .../core/connection/ConnectionManager.java | 83 +++++++++------- .../core/connection/IpmiMessageHandler.java | 18 ++-- .../ipmi/core/connection/SessionManager.java | 15 ++- .../core/connection/queue/MessageQueue.java | 4 +- .../metricshub/ipmi/core/sm/StateMachine.java | 4 +- src/main/resources/connection.properties | 2 - src/site/markdown/low-level-api.md | 2 +- src/site/markdown/upgrading.md | 13 ++- .../core/common/PropertiesManagerTest.java | 17 ++++ .../connection/ConnectionManagerTest.java | 75 +++++++++++++++ .../ipmi/core/connection/ConnectionTest.java | 96 +++++++++++++++++++ .../core/connection/SessionManagerTest.java | 52 ++++++++++ 15 files changed, 359 insertions(+), 92 deletions(-) create mode 100644 src/test/java/org/metricshub/ipmi/core/common/PropertiesManagerTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/connection/SessionManagerTest.java diff --git a/src/main/java/org/metricshub/ipmi/core/common/Constants.java b/src/main/java/org/metricshub/ipmi/core/common/Constants.java index c67467f..73b46bd 100644 --- a/src/main/java/org/metricshub/ipmi/core/common/Constants.java +++ b/src/main/java/org/metricshub/ipmi/core/common/Constants.java @@ -32,7 +32,5 @@ public final class Constants { */ public static final int IPMI_PORT = 0x26F; - public static final int TIMEOUT = 500; - private Constants() {} } diff --git a/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java b/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java index 2962707..79697bd 100644 --- a/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java +++ b/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java @@ -26,15 +26,16 @@ 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 properties; @@ -42,7 +43,7 @@ public final class PropertiesManager { private Logger logger = LoggerFactory.getLogger(PropertiesManager.class); private PropertiesManager() { - properties = new HashMap(); + properties = new ConcurrentHashMap(); loadProperties("/connection.properties"); loadProperties("/vxipmi.properties"); @@ -50,14 +51,25 @@ private PropertiesManager() { @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); } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index cdc2f91..862772e 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/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,27 @@ public void notify(StateMachineAction action) { } /** - * {@link TimerTask} runner - periodically sends no-op messages to keep the - * session up + * {@link TimerTask} runner - periodically sends a no-op message to keep the session up. The message is sent + * one-way: its reply is not awaited, so it is neither queued nor retried, and a reply to the same command sent by + * the application is delivered to the application. */ @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 GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + ((SessionValid) current).getCipherSuite(), + PrivilegeLevel.Callback, + TypeConverter.intToByte(0xe)), + true); + } catch (Exception e) { + LOGGER.error("Keep-alive failed: " + e.getMessage(), e); } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index c3b8198..a501e76 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -44,7 +44,7 @@ public class ConnectionManager { private static final Object SESSIONLESS_TAG_LOCK = new Object(); private static int sessionlessTag; - private static List reservedTags = new ArrayList(); + private static final List RESERVED_TAGS = new ArrayList(); /** * Frequency of the no-op commands that will be sent to keep up the session @@ -60,8 +60,10 @@ public class ConnectionManager { * @throws IOException If UdpMessenger encountered an error */ public ConnectionManager(int port, long pingPeriod) throws IOException { - this(port); + messenger = new UdpMessenger(port); + // Set before initialize(), which resolves -1 to the connection.properties period this.pingPeriod = pingPeriod; + initialize(); } /** @@ -103,12 +105,20 @@ public ConnectionManager(Messenger messenger) { private void initialize() { connections = new ArrayList(); - reservedTags = new ArrayList(); if (pingPeriod == -1) { pingPeriod = Long.parseLong(PropertiesManager.getInstance().getProperty("pingPeriod")); } } + /** + * Returns the keep-alive period of the connections created without an explicit one. + * + * @return the period in ms between two keep-alive messages, 0 or negative when the sessions are not kept alive + */ + public long getPingPeriod() { + return pingPeriod; + } + /** * Closes all open connections and disconnects {@link UdpListener}. */ @@ -135,8 +145,8 @@ public static int generateSessionlessTag() { boolean interrupted = false; while (wait) { sessionlessTag = (sessionlessTag + 1) % 60; - synchronized (reservedTags) { - if (!reservedTags.contains(sessionlessTag)) { + synchronized (RESERVED_TAGS) { + if (!RESERVED_TAGS.contains(sessionlessTag)) { wait = false; } } @@ -148,8 +158,8 @@ public static int generateSessionlessTag() { } } } - synchronized (reservedTags) { - reservedTags.add(sessionlessTag); + synchronized (RESERVED_TAGS) { + RESERVED_TAGS.add(sessionlessTag); } if (interrupted) { Thread.currentThread().interrupt(); @@ -165,8 +175,8 @@ public static int generateSessionlessTag() { * - tag to free */ public static void freeTag(int tag) { - synchronized (reservedTags) { - reservedTags.remove((Integer) tag); + synchronized (RESERVED_TAGS) { + RESERVED_TAGS.remove((Integer) tag); } } @@ -177,14 +187,28 @@ public static void freeTag(int tag) { * - index of the connection to return */ public Connection getConnection(int index) { - return connections.get(index); + Connection connection; + synchronized (connections) { + connection = connections.get(index); + } + if (connection == null) { + throw new IllegalStateException("Connection " + index + " is closed"); + } + return connection; } /** - * Closes the connection with the given index. + * Closes the connection with the given index and releases it; the index is not reused. Closing an already + * closed connection does nothing. */ public void closeConnection(int index) { - connections.get(index).disconnect(); + Connection connection; + synchronized (connections) { + connection = connections.set(index, null); + } + if (connection != null) { + connection.disconnect(); + } } /** @@ -200,7 +224,7 @@ public Connection getConnection(InetAddress address, int port) { for (Connection connection : connections) { if (connection != null && connection.isActive() - && connection.getRemoteMachineAddress() == address + && connection.getRemoteMachineAddress().equals(address) && connection.getRemoteMachinePort() == port) { return connection; } @@ -225,10 +249,14 @@ public Connection getConnection(InetAddress address, int port) { */ public int createConnection(InetAddress address, int port, int connectionPingPeriod, boolean skipCiphers) throws IOException { - Connection connection = new Connection(messenger, 0); - connection.connect(address, port, connectionPingPeriod, skipCiphers); + return connect(address, port, connectionPingPeriod, skipCiphers); + } + private int connect(InetAddress address, int port, long connectionPingPeriod, boolean skipCiphers) + throws IOException { synchronized (connections) { + Connection connection = new Connection(messenger, connections.size()); + connection.connect(address, port, connectionPingPeriod, skipCiphers); connections.add(connection); return connections.size() - 1; } @@ -247,13 +275,7 @@ public int createConnection(InetAddress address, int port, int connectionPingPer * - when properties file was not found */ public int createConnection(InetAddress address, int port, int connectionPingPeriod) throws IOException { - Connection connection = new Connection(messenger, 0); - connection.connect(address, port, connectionPingPeriod); - - synchronized (connections) { - connections.add(connection); - return connections.size() - 1; - } + return connect(address, port, connectionPingPeriod, false); } /** @@ -267,15 +289,7 @@ public int createConnection(InetAddress address, int port, int connectionPingPer * when properties file was not found */ public int createConnection(InetAddress address, int port) throws IOException { - - synchronized (connections) { - Connection connection = new Connection( - messenger, - connections.size()); - connection.connect(address, port, pingPeriod); - connections.add(connection); - return connections.size() - 1; - } + return connect(address, port, pingPeriod, false); } /** @@ -291,12 +305,7 @@ public int createConnection(InetAddress address, int port) throws IOException { * when properties file was not found */ public int createConnection(InetAddress address, int port, boolean skipCiphers) throws IOException { - synchronized (connections) { - Connection connection = new Connection(messenger, connections.size()); - connection.connect(address, port, pingPeriod, skipCiphers); - connections.add(connection); - return connections.size() - 1; - } + return connect(address, port, pingPeriod, skipCiphers); } /** diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index d44f9dc..230191b 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -24,7 +24,6 @@ import org.metricshub.ipmi.core.coding.PayloadCoder; import org.metricshub.ipmi.core.coding.commands.ResponseData; -import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilities; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; @@ -70,18 +69,13 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } - if (coder.getClass() == GetChannelAuthenticationCapabilities.class) { - getMessageQueue().remove(tag); - } else { - - try { - ResponseData responseData = coder.getResponseData(message); - getConnection().notifyResponseListeners(getConnection().getHandle(), tag, responseData, null); - } catch (Exception e) { - getConnection().notifyResponseListeners(getConnection().getHandle(), tag, null, e); - } - getMessageQueue().remove(lanMessagePayload.getSequenceNumber()); + try { + ResponseData responseData = coder.getResponseData(message); + getConnection().notifyResponseListeners(getConnection().getHandle(), tag, responseData, null); + } catch (Exception e) { + getConnection().notifyResponseListeners(getConnection().getHandle(), tag, null, e); } + getMessageQueue().remove(tag); } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java index d74e5d1..3920afb 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java @@ -83,15 +83,20 @@ public static Session establishSession( } } + /** + * Closes the session and the connection that failed to establish a session, leaving the connector and its other + * connections untouched. + */ private static void closeConnection(IpmiConnector connector, ConnectionHandle handle) { + if (connector == null || handle == null) { + return; + } try { - if (connector != null && handle != null) { - connector.closeSession(handle); - connector.tearDown(); - } + connector.closeSession(handle); } catch (Exception e) { - LOGGER.error("Cannot close connection after exception thrown during session establishment.", e); + LOGGER.error("Cannot close session after exception thrown during session establishment.", e); } + connector.closeConnection(handle); } private final ConcurrentHashMap sessionsPerConnectionHandle; diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java index 4880eda..bcdabbe 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java @@ -54,7 +54,7 @@ public class MessageQueue extends TimerTask { /** * Frequency of checking messages for timeouts in ms. */ - private static int cleaningFrequency = 500; + private static final int CLEANING_FREQUENCY = 500; /** * Size of the queue determined by IPMI sliding window algorithm @@ -80,7 +80,7 @@ public MessageQueue(Connection connection, int timeout, int minSequenceNumber, i queue = new ArrayList(); setTimeout(timeout); timer = new Timer(true); - timer.schedule(this, cleaningFrequency, cleaningFrequency); + timer.schedule(this, CLEANING_FREQUENCY, CLEANING_FREQUENCY); } private int incrementSequenceNumber(int currentSequenceNumber) { diff --git a/src/main/java/org/metricshub/ipmi/core/sm/StateMachine.java b/src/main/java/org/metricshub/ipmi/core/sm/StateMachine.java index e5e3a4a..a9890bb 100644 --- a/src/main/java/org/metricshub/ipmi/core/sm/StateMachine.java +++ b/src/main/java/org/metricshub/ipmi/core/sm/StateMachine.java @@ -132,13 +132,15 @@ public void start(InetAddress address, int port) { } /** - * Cleans up the machine resources. + * Cleans up the machine resources and leaves the current state: a stopped machine no longer reports a valid + * session. * * @see #start(InetAddress, int) */ public void stop() { messenger.unregister(this); initialized = false; + current = new Uninitialized(); } /** diff --git a/src/main/resources/connection.properties b/src/main/resources/connection.properties index 785e798..69dae4a 100644 --- a/src/main/resources/connection.properties +++ b/src/main/resources/connection.properties @@ -2,5 +2,3 @@ pingPeriod=30000 #Time in ms after which a message times out. timeout=5000 -#Frequency of checking messages for timeouts in ms. -cleaningFrequency=500 diff --git a/src/site/markdown/low-level-api.md b/src/site/markdown/low-level-api.md index 69729c2..9a109ea 100644 --- a/src/site/markdown/low-level-api.md +++ b/src/site/markdown/low-level-api.md @@ -94,7 +94,7 @@ port (or always pass `0`), and call `tearDown()` when you are done with it. | `createConnection(InetAddress address[, int port])` | Register a connection to a BMC (port 623 by default). | | `createConnection(InetAddress address, [int port,] CipherSuite cipherSuite, PrivilegeLevel level)` | The same, skipping the cipher suite and capabilities steps: call `openSession()` next. | | `closeSession(handle)` | Log out (Close Session). | -| `closeConnection(handle)` | Forget the connection. | +| `closeConnection(handle)` | Close and release the connection; the handle is no longer usable. | | `tearDown()` | Close every connection and release the local port. | ### Choosing the cipher suite diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 21c1e37..53433d1 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -36,7 +36,18 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world check failed`, `IPMIException: Unauthorized name.`), where 1.2.02 sent the credentials four times and threw `ConnectionException: Illegal connection state: Rakp1Waiting` ([Troubleshooting](troubleshooting.html#the-login-fails)); only a handshake step that got no - reply is sent again. + reply is sent again; +* the [keep-alive](configuration.html#keep-alive) is actually sent: with the default `pingPeriod` + (`-1`) 1.2.02 sent no keep-alive at all, so a session could expire during a long collection; the + keep-alive is now one message every 30 s by default, sent without waiting for its reply, and a + Get Channel Authentication Capabilities command sent by the application in a session gets its + reply (1.2.02 dropped it, as it did the keep-alive replies); +* `IpmiConnector.closeConnection()` releases the connection: its handle then throws + `IllegalStateException` instead of addressing a disconnected connection, and a session that + fails to be established by `SerialOverLan` closes its own connection instead of tearing down the + whole connector; +* the `PropertiesManager` lookups are logged at `DEBUG` instead of `INFO`, and the unused + `cleaningFrequency` property is gone from `connection.properties`. The decoders follow the IPMI 2.0 and FRU specifications more closely; the visible changes are: diff --git a/src/test/java/org/metricshub/ipmi/core/common/PropertiesManagerTest.java b/src/test/java/org/metricshub/ipmi/core/common/PropertiesManagerTest.java new file mode 100644 index 0000000..a367c79 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/common/PropertiesManagerTest.java @@ -0,0 +1,17 @@ +package org.metricshub.ipmi.core.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; + +class PropertiesManagerTest { + + @Test + void aMissingResourceIsSkipped() { + PropertiesManager manager = PropertiesManager.getInstance(); + manager.loadProperties("/does-not-exist.properties"); + assertEquals("30000", manager.getProperty("pingPeriod"), "the packaged properties must still be there"); + assertNull(manager.getProperty("cleaningFrequency"), "cleaningFrequency is not a setting"); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java index 62aee05..abcccec 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java @@ -8,6 +8,10 @@ import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import java.net.InetAddress; +import org.metricshub.ipmi.core.transport.SilentMessenger; class ConnectionManagerTest { @@ -43,4 +47,75 @@ void generateSessionlessTagKeepsWaitingWhenInterruptedOnSaturatedPool() throws E } } } + + @Test + void pingPeriodComesFromThePropertiesOnlyWhenNotGiven() throws Exception { + assertPingPeriod(30000, new ConnectionManager(0)); + assertPingPeriod(30000, new ConnectionManager(0, -1)); + assertPingPeriod(12345, new ConnectionManager(0, 12345)); + assertPingPeriod(0, new ConnectionManager(0, 0)); + } + + private static void assertPingPeriod(long expected, ConnectionManager manager) { + try { + assertEquals(expected, manager.getPingPeriod()); + } finally { + manager.close(); + } + } + + @Test + void sessionlessTagsStayReservedWhenAnotherManagerIsCreated() { + int reserved = ConnectionManager.generateSessionlessTag(); + int[] others = new int[TAG_COUNT - 1]; + try { + new ConnectionManager(new SilentMessenger()).close(); + for (int i = 0; i < others.length; i++) { + others[i] = ConnectionManager.generateSessionlessTag(); + assertNotEquals(reserved, others[i], "a tag reserved before the second manager was handed out again"); + } + } finally { + ConnectionManager.freeTag(reserved); + for (int tag : others) { + ConnectionManager.freeTag(tag); + } + } + } + + @Test + void everyCreateConnectionOverloadReturnsTheHandleOfTheConnection() throws Exception { + ConnectionManager manager = new ConnectionManager(new SilentMessenger()); + InetAddress bmc = InetAddress.getLoopbackAddress(); + try { + int[] handles = { + manager.createConnection(bmc, 623), + manager.createConnection(bmc, 623, 0), + manager.createConnection(bmc, 623, 0, true), + manager.createConnection(bmc, 623, true) }; + for (int i = 0; i < handles.length; i++) { + assertEquals(i, handles[i]); + assertEquals(i, manager.getConnection(i).getHandle(), "the connection must carry its own handle"); + } + } finally { + manager.close(); + } + } + + @Test + void closeConnectionReleasesTheConnectionAndKeepsTheOtherHandles() throws Exception { + ConnectionManager manager = new ConnectionManager(new SilentMessenger()); + InetAddress bmc = InetAddress.getLoopbackAddress(); + try { + int first = manager.createConnection(bmc, 623); + int second = manager.createConnection(bmc, 623); + manager.closeConnection(first); + assertThrows(IllegalStateException.class, () -> manager.getConnection(first)); + assertEquals(second, manager.getConnection(second).getHandle()); + assertTrue(manager.getConnection(second).isActive()); + manager.closeConnection(first); // closing twice is harmless + assertEquals(2, manager.createConnection(bmc, 623), "a released handle is not reused"); + } finally { + manager.close(); + } + } } diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index a8dd717..5e2b387 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -16,6 +16,22 @@ import org.junit.jupiter.api.Test; import org.metricshub.ipmi.core.api.sync.IpmiConnector; import org.metricshub.ipmi.core.transport.SilentMessenger; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.lang.reflect.Field; +import java.util.concurrent.atomic.AtomicInteger; +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.payload.IpmiPayload; +import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse; +import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; +import org.metricshub.ipmi.core.coding.protocol.PayloadType; +import org.metricshub.ipmi.core.coding.security.CipherSuite; +import org.metricshub.ipmi.core.sm.StateMachine; +import org.metricshub.ipmi.core.sm.actions.MessageAction; +import org.metricshub.ipmi.core.sm.states.SessionValid; +import org.metricshub.ipmi.core.transport.UdpMessage; class ConnectionTest { @@ -103,4 +119,84 @@ void libraryThreadsAreDaemonThreads() throws Exception { connector.tearDown(); } } + + /** + * Puts the connection in the session-open state without a handshake (there is no BMC behind the messenger). + */ + private static void openSession(Connection connection) throws Exception { + Field field = Connection.class.getDeclaredField("stateMachine"); + field.setAccessible(true); + ((StateMachine) field.get(connection)).setCurrent(new SessionValid(CipherSuite.getEmpty(), 1)); + } + + @Test + void keepAliveSendsOneMessageAndReturnsAtOnce() throws Exception { + AtomicInteger sent = new AtomicInteger(); + Connection connection = new Connection(new SilentMessenger() { + @Override + public void send(UdpMessage message) { + sent.incrementAndGet(); + } + }, 0); + connection.connect(InetAddress.getLoopbackAddress(), 623, 0); + try { + openSession(connection); + long start = System.nanoTime(); + connection.run(); + long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + assertEquals(1, sent.get()); + assertTrue(elapsed < 500, "the keep-alive must not sleep, took " + elapsed + " ms"); + + connection.disconnect(); + assertFalse(connection.isSessionValid(), "a disconnected connection has no session"); + connection.run(); + assertEquals(1, sent.get(), "a disconnected connection sends no keep-alive"); + } finally { + connection.disconnect(); + } + } + + @Test + void aGetChannelAuthenticationCapabilitiesReplyReachesTheListeners() throws Exception { + Connection connection = connect(TIMEOUT_MS); + try { + openSession(connection); + AtomicInteger notifiedTag = new AtomicInteger(-1); + AtomicReference outcome = new AtomicReference<>(); + connection.registerListener(new ConnectionListener() { + @Override + public void processResponse(ResponseData responseData, int handle, int tag, Exception exception) { + notifiedTag.set(tag); + outcome.set(responseData != null ? responseData : exception); + } + + @Override + public void processRequest(IpmiPayload payload) { + // not expected + } + }); + GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + CipherSuite.getEmpty(), + PrivilegeLevel.Callback, + (byte) 0xe); + int tag = connection.sendMessage(request, false); + assertTrue(tag > 0, "the request must be queued"); + + // A minimal response carrying the tag of the request (rqSeq, bits 7:2 of byte 4) + byte[] raw = { 0x20, 0x18, 0, (byte) 0x81, (byte) (tag << 2), 0x38, 0, 0 }; + raw[2] = (byte) -(raw[0] + raw[1]); + raw[7] = (byte) -(raw[3] + raw[4] + raw[5] + raw[6]); + Ipmiv20Message reply = new Ipmiv20Message(null); + reply.setPayloadType(PayloadType.Ipmi); + reply.setPayload(new IpmiLanResponse(raw)); + connection.notify(new MessageAction(reply)); + + assertEquals(tag, notifiedTag.get(), "the reply must be delivered to the listeners"); + assertNotNull(outcome.get()); + } finally { + connection.disconnect(); + } + } } diff --git a/src/test/java/org/metricshub/ipmi/core/connection/SessionManagerTest.java b/src/test/java/org/metricshub/ipmi/core/connection/SessionManagerTest.java new file mode 100644 index 0000000..c833ef1 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/connection/SessionManagerTest.java @@ -0,0 +1,52 @@ +package org.metricshub.ipmi.core.connection; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.api.async.ConnectionHandle; +import org.metricshub.ipmi.core.api.sol.CipherSuiteSelectionHandler; +import org.metricshub.ipmi.core.api.sync.IpmiConnector; +import org.metricshub.ipmi.core.common.PropertiesManager; +import org.metricshub.ipmi.core.transport.FakeBmc; + +class SessionManagerTest { + + private static final int TIMEOUT_MS = 200; + + @Test + void aFailedSessionClosesItsConnectionOnly() throws Exception { + PropertiesManager properties = PropertiesManager.getInstance(); + String timeout = properties.getProperty("timeout"); + properties.setProperty("timeout", String.valueOf(TIMEOUT_MS)); + try (FakeBmc bmc = FakeBmc.silent()) { + IpmiConnector connector = new IpmiConnector(0); + try { + ConnectionHandle other = connector.createConnection(bmc.getAddress(), bmc.getPort()); + CipherSuiteSelectionHandler firstSuite = suites -> suites.isEmpty() ? null : suites.get(0); + assertThrows( + SessionException.class, + () -> SessionManager + .establishSession( + connector, + bmc.getAddress().getHostAddress(), + bmc.getPort(), + "user", + "password", + firstSuite)); + + // The failed connection (handle 1) is closed; the other one still sends and times out, instead of + // being told that the socket is closed + assertThrows(IllegalStateException.class, () -> connector.getTimeout(new ConnectionHandle(1, null, 0))); + ConnectionException e = assertThrows( + ConnectionException.class, + () -> connector.getAvailableCipherSuites(other)); + assertEquals("Command timed out", e.getMessage()); + } finally { + connector.tearDown(); + } + } finally { + properties.setProperty("timeout", timeout); + } + } +} From 82468183815433b304eb19d234bbaced6b05f37d Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:01:54 +0200 Subject: [PATCH 2/8] Address the review of the lifecycle fixes A one-way message skips the tags of the queued requests, so the keep-alive can never take the tag of a pending request; every operation of ConnectionManager on a released handle throws IllegalStateException; getPingPeriod() is package-private; Constants.TIMEOUT is deprecated instead of removed; the documentation of the property lookups says DEBUG and thread-safe. Co-Authored-By: Claude Fable 5.1 --- .../ipmi/core/common/Constants.java | 8 ++++++++ .../core/connection/ConnectionManager.java | 12 +++++------ .../core/connection/queue/MessageQueue.java | 8 +++++--- src/site/markdown/installation.md | 7 +++---- src/site/markdown/timeouts-and-errors.md | 6 +++--- src/site/markdown/upgrading.md | 5 +++-- .../connection/ConnectionManagerTest.java | 18 +++++++++++++++++ .../connection/queue/MessageQueueTest.java | 20 +++++++++++++++++++ 8 files changed, 65 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/common/Constants.java b/src/main/java/org/metricshub/ipmi/core/common/Constants.java index 73b46bd..ad9a3d0 100644 --- a/src/main/java/org/metricshub/ipmi/core/common/Constants.java +++ b/src/main/java/org/metricshub/ipmi/core/common/Constants.java @@ -32,5 +32,13 @@ 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() {} } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index a501e76..b760af2 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -115,7 +115,7 @@ private void initialize() { * * @return the period in ms between two keep-alive messages, 0 or negative when the sessions are not kept alive */ - public long getPingPeriod() { + long getPingPeriod() { return pingPeriod; } @@ -326,7 +326,7 @@ public List getAvailableCipherSuites(int connection) int tag = generateSessionlessTag(); List suites; try { - suites = connections.get(connection).getAvailableCipherSuites(tag); + suites = getConnection(connection).getAvailableCipherSuites(tag); } catch (Exception e) { freeTag(tag); throw e; @@ -361,8 +361,7 @@ public GetChannelAuthenticationCapabilitiesResponseData getChannelAuthentication int tag = generateSessionlessTag(); GetChannelAuthenticationCapabilitiesResponseData responseData; try { - responseData = connections - .get(connection) + responseData = getConnection(connection) .getChannelAuthenticationCapabilities( tag, cipherSuite, @@ -411,8 +410,7 @@ public int startSession( int sessionId; int tag = generateSessionlessTag(); try { - sessionId = connections - .get(connection) + sessionId = getConnection(connection) .startSession( tag, cipherSuite, @@ -438,6 +436,6 @@ public int startSession( * - {@link ConnectionListener} to processResponse */ public void registerListener(int connection, ConnectionListener listener) { - connections.get(connection).registerListener(listener); + getConnection(connection).registerListener(listener); } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java index bcdabbe..74c4335 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java @@ -240,14 +240,16 @@ public boolean containsId(int sequenceNumber) { } /** - * Returns valid session sequence number that cannot be used as a tag though + * Returns the sequence number for a message that awaits no reply: it skips the tags of the queued requests, so + * a reply to the one-way message can never be taken for the reply of a queued request. */ public int getSequenceNumber() { synchronized (lastSequenceNumberLock) { int sequenceNumber = incrementSequenceNumber(lastSequenceNumber); - + while (isReserved(sequenceNumber)) { + sequenceNumber = incrementSequenceNumber(sequenceNumber); + } lastSequenceNumber = sequenceNumber; - return sequenceNumber; } } diff --git a/src/site/markdown/installation.md b/src/site/markdown/installation.md index fa283c3..d12c905 100644 --- a/src/site/markdown/installation.md +++ b/src/site/markdown/installation.md @@ -59,12 +59,11 @@ What is logged, and at which level: | --- | --- | | `ERROR` | A value the decoders do not know (`Invalid value: ...` for an entity ID, sensor type or unit), a failed session handshake, exceptions in the receiving and keep-alive threads. | | `WARN` | An SDR record that cannot be decoded and is skipped, a FRU that cannot be read or decoded (the FRU is then truncated or missing), a message that failed and is resent, a packet whose integrity check failed, a response listener that threw while a timeout was reported. | -| `INFO` | Every lookup of a [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) value. | -| `DEBUG` | Each message sent, with its tag and attempt number; each message that timed out; a session that could not be closed cleanly. | +| `DEBUG` | Each message sent, with its tag and attempt number; each message that timed out; a session that could not be closed cleanly; every lookup of a [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) value. | > [!TIP] -> The `INFO` messages are noisy: set the `org.metricshub.ipmi` logger to `WARN` in production, -> and to `DEBUG` when diagnosing a BMC ([Troubleshooting](troubleshooting.html)). +> Set the `org.metricshub.ipmi` logger to `WARN` in production, and to `DEBUG` when diagnosing a +> BMC ([Troubleshooting](troubleshooting.html)). With `slf4j-simple`, for example: diff --git a/src/site/markdown/timeouts-and-errors.md b/src/site/markdown/timeouts-and-errors.md index 31c1f83..d59b143 100644 --- a/src/site/markdown/timeouts-and-errors.md +++ b/src/site/markdown/timeouts-and-errors.md @@ -76,9 +76,9 @@ properties.setProperty("timeout", "2000"); // per-message timeout: 2 s instead o properties.setProperty("retries", "3"); ``` -`PropertiesManager` logs every lookup at the `INFO` level and its lazy initialization is not -synchronized ([#98](https://github.com/metricshub/ipmi-java/issues/98)), hence "from a single -thread, at startup". +`PropertiesManager` logs every lookup at the `DEBUG` level. Its initialization is thread-safe and +`setProperty()` may be called at any time, but a value changed while connections are being created +applies to some of them and not to others, hence "from a single thread, at startup". ## Exceptions diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 53433d1..af3f1bb 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -46,8 +46,9 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world `IllegalStateException` instead of addressing a disconnected connection, and a session that fails to be established by `SerialOverLan` closes its own connection instead of tearing down the whole connector; -* the `PropertiesManager` lookups are logged at `DEBUG` instead of `INFO`, and the unused - `cleaningFrequency` property is gone from `connection.properties`. +* the `PropertiesManager` lookups are logged at `DEBUG` instead of `INFO`, the unused + `cleaningFrequency` property is gone from `connection.properties`, and `Constants.TIMEOUT`, + which nothing reads, is deprecated. The decoders follow the IPMI 2.0 and FRU specifications more closely; the visible changes are: diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java index abcccec..4ac6667 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java @@ -118,4 +118,22 @@ void closeConnectionReleasesTheConnectionAndKeepsTheOtherHandles() throws Except manager.close(); } } + + @Test + void everyOperationOnAReleasedHandleFailsTheSameWay() throws Exception { + ConnectionManager manager = new ConnectionManager(new SilentMessenger()); + try { + int handle = manager.createConnection(InetAddress.getLoopbackAddress(), 623); + manager.closeConnection(handle); + assertThrows(IllegalStateException.class, () -> manager.getAvailableCipherSuites(handle)); + assertThrows( + IllegalStateException.class, + () -> manager.getChannelAuthenticationCapabilities(handle, null, null)); + assertThrows(IllegalStateException.class, () -> manager.startSession(handle, null, null, "", "", null)); + assertThrows(IllegalStateException.class, () -> manager.registerListener(handle, null)); + assertThrows(IllegalStateException.class, () -> manager.getConnection(handle)); + } finally { + manager.close(); + } + } } diff --git a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java index 3e903bc..7d2699e 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java @@ -115,4 +115,24 @@ public void processRequest(IpmiPayload payload) { queue.tearDown(); } } + + @Test + void oneWaySequenceNumbersSkipTheQueuedTags() { + MessageQueue queue = newQueue(); + try { + int queued = queue.add(request()); + assertTrue(queued > 0); + // Twice around the 63-value sequence space + for (int i = 0; i < 2 * IpmiLanMessage.MAX_SEQUENCE_NUMBER; i++) { + int sequenceNumber = queue.getSequenceNumber(); + assertTrue(sequenceNumber != queued, "a one-way message took the tag of the queued request"); + assertTrue( + sequenceNumber >= IpmiLanMessage.MIN_SEQUENCE_NUMBER + && sequenceNumber <= IpmiLanMessage.MAX_SEQUENCE_NUMBER, + "out of range: " + sequenceNumber); + } + } finally { + queue.tearDown(); + } + } } From b786faa3584c1c2468ba8f6b82d3bf756a7467d6 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:11:42 +0200 Subject: [PATCH 3/8] Drop stale replies; look connections up without a lock IpmiMessageHandler drops a reply whose command is not the command of the queued request with that tag: a late reply to the one-way keep-alive can no longer be decoded as the reply of a request that was given its sequence number since. ConnectionManager keeps its connections in a copy-on-write list looked up without a lock: the receiving thread, which holds the UDP listener lock, no longer waits for a lock that connect() and close() hold while registering or unregistering UDP listeners. Co-Authored-By: Claude Fable 5.1 --- .../core/connection/ConnectionManager.java | 40 +++++++--------- .../core/connection/IpmiMessageHandler.java | 12 +++++ .../ipmi/core/connection/ConnectionTest.java | 48 +++++++++++++++++++ 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index b760af2..a668c80 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -34,13 +34,17 @@ import java.net.InetAddress; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; /** * Manages multiple {@link Connection}s */ public class ConnectionManager { private Messenger messenger; + // Copy-on-write: looked up without a lock by the receiving thread, which holds a state machine lock, while + // connect() takes a state machine lock under the list lock private List connections; + private final Object connectionsLock = new Object(); private static final Object SESSIONLESS_TAG_LOCK = new Object(); private static int sessionlessTag; @@ -104,7 +108,7 @@ public ConnectionManager(Messenger messenger) { } private void initialize() { - connections = new ArrayList(); + connections = new CopyOnWriteArrayList(); if (pingPeriod == -1) { pingPeriod = Long.parseLong(PropertiesManager.getInstance().getProperty("pingPeriod")); } @@ -123,11 +127,9 @@ long getPingPeriod() { * Closes all open connections and disconnects {@link UdpListener}. */ public void close() { - synchronized (connections) { - for (Connection connection : connections) { - if (connection != null && connection.isActive()) { - connection.disconnect(); - } + for (Connection connection : connections) { + if (connection != null && connection.isActive()) { + connection.disconnect(); } } messenger.closeConnection(); @@ -187,10 +189,7 @@ public static void freeTag(int tag) { * - index of the connection to return */ public Connection getConnection(int index) { - Connection connection; - synchronized (connections) { - connection = connections.get(index); - } + Connection connection = connections.get(index); if (connection == null) { throw new IllegalStateException("Connection " + index + " is closed"); } @@ -202,10 +201,7 @@ public Connection getConnection(int index) { * closed connection does nothing. */ public void closeConnection(int index) { - Connection connection; - synchronized (connections) { - connection = connections.set(index, null); - } + Connection connection = connections.set(index, null); if (connection != null) { connection.disconnect(); } @@ -220,14 +216,12 @@ public void closeConnection(int index) { * @return First {@link Connection} to the address or null if none found */ public Connection getConnection(InetAddress address, int port) { - synchronized (connections) { - for (Connection connection : connections) { - if (connection != null - && connection.isActive() - && connection.getRemoteMachineAddress().equals(address) - && connection.getRemoteMachinePort() == port) { - return connection; - } + for (Connection connection : connections) { + if (connection != null + && connection.isActive() + && connection.getRemoteMachineAddress().equals(address) + && connection.getRemoteMachinePort() == port) { + return connection; } } return null; @@ -254,7 +248,7 @@ public int createConnection(InetAddress address, int port, int connectionPingPer private int connect(InetAddress address, int port, long connectionPingPeriod, boolean skipCiphers) throws IOException { - synchronized (connections) { + synchronized (connectionsLock) { Connection connection = new Connection(messenger, connections.size()); connection.connect(address, port, connectionPingPeriod, skipCiphers); connections.add(connection); diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index 230191b..9095722 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -23,6 +23,7 @@ */ import org.metricshub.ipmi.core.coding.PayloadCoder; +import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder; import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; @@ -69,6 +70,17 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } + // A reply to a message whose reply was not awaited (the keep-alive) may arrive once its sequence number + // has been given to a queued request: the reply of a request answers the command of that request + if (coder instanceof IpmiCommandCoder + && ((IpmiCommandCoder) coder).getCommandCode() != lanMessagePayload.getCommand()) { + LOGGER + .debug( + "Message tagged with " + tag + " answers command " + lanMessagePayload.getCommand() + + ", not the queued request. Dropping stale message."); + return; + } + try { ResponseData responseData = coder.getResponseData(message); getConnection().notifyResponseListeners(getConnection().getHandle(), tag, responseData, null); diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index 5e2b387..1ade6ad 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -199,4 +199,52 @@ public void processRequest(IpmiPayload payload) { connection.disconnect(); } } + + @Test + void aReplyToAnotherCommandWithTheTagOfAQueuedRequestIsDropped() throws Exception { + Connection connection = connect(TIMEOUT_MS); + try { + openSession(connection); + AtomicInteger notifiedTag = new AtomicInteger(-1); + connection.registerListener(new ConnectionListener() { + @Override + public void processResponse(ResponseData responseData, int handle, int tag, Exception exception) { + notifiedTag.set(tag); + } + + @Override + public void processRequest(IpmiPayload payload) { + // not expected + } + }); + GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + CipherSuite.getEmpty(), + PrivilegeLevel.Callback, + (byte) 0xe); + int tag = connection.sendMessage(request, false); + + // A late reply to a one-way Get Device ID (command 01h) whose sequence number was reused by the request + connection.notify(new MessageAction(reply(tag, (byte) 0x01))); + assertEquals(-1, notifiedTag.get(), "a reply to another command must not answer the queued request"); + + // The request is still queued: its own reply (command 38h) is delivered + connection.notify(new MessageAction(reply(tag, (byte) 0x38))); + assertEquals(tag, notifiedTag.get()); + } finally { + connection.disconnect(); + } + } + + /** A minimal IPMI LAN response with the given tag (rqSeq, bits 7:2 of byte 4) and command. */ + private static Ipmiv20Message reply(int tag, byte command) { + byte[] raw = { 0x20, 0x18, 0, (byte) 0x81, (byte) (tag << 2), command, 0, 0 }; + raw[2] = (byte) -(raw[0] + raw[1]); + raw[7] = (byte) -(raw[3] + raw[4] + raw[5] + raw[6]); + Ipmiv20Message reply = new Ipmiv20Message(null); + reply.setPayloadType(PayloadType.Ipmi); + reply.setPayload(new IpmiLanResponse(raw)); + return reply; + } } From 7312ecf74b044ceab15cd3fd2cf1447565407d47 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:22:45 +0200 Subject: [PATCH 4/8] Queue the keep-alive as a marked request; close the manager atomically The keep-alive is a Connection.KeepAlive request, queued like any request so that its tag stays reserved until its reply arrives or it times out; IpmiMessageHandler discards the reply of a KeepAlive and delivers every other reply, including the application's own Get Channel Authentication Capabilities. The command-code check is gone. ConnectionManager.close() disconnects the connections under the same lock as connect() and marks the manager closed; connect() then throws IllegalStateException instead of creating a connection on a closed messenger. Co-Authored-By: Claude Fable 5.1 --- .../ipmi/core/connection/Connection.java | 25 +++++++++++-------- .../core/connection/ConnectionManager.java | 13 +++++++--- .../core/connection/IpmiMessageHandler.java | 12 +++------ src/site/markdown/upgrading.md | 2 +- .../connection/ConnectionManagerTest.java | 11 ++++++++ .../ipmi/core/connection/ConnectionTest.java | 16 ++++++------ 6 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index 862772e..1a41cda 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java @@ -668,9 +668,19 @@ public void notify(StateMachineAction action) { } /** - * {@link TimerTask} runner - periodically sends a no-op message to keep the session up. The message is sent - * one-way: its reply is not awaited, so it is neither queued nor retried, and a reply to the same command sent by - * the application is delivered to the application. + * The keep-alive request: queued like any request, so that its tag stays reserved until its reply arrives or it + * times out, and recognized by {@link IpmiMessageHandler}, which discards its reply. + */ + static final class KeepAlive extends GetChannelAuthenticationCapabilities { + 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() { @@ -679,14 +689,7 @@ public void run() { return; } try { - sendMessage( - new GetChannelAuthenticationCapabilities( - IpmiVersion.V20, - IpmiVersion.V20, - ((SessionValid) current).getCipherSuite(), - PrivilegeLevel.Callback, - TypeConverter.intToByte(0xe)), - true); + sendMessage(new KeepAlive(((SessionValid) current).getCipherSuite()), false); } catch (Exception e) { LOGGER.error("Keep-alive failed: " + e.getMessage(), e); } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index a668c80..cf39a31 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -45,6 +45,7 @@ public class ConnectionManager { // connect() takes a state machine lock under the list lock private List connections; private final Object connectionsLock = new Object(); + private boolean closed; private static final Object SESSIONLESS_TAG_LOCK = new Object(); private static int sessionlessTag; @@ -127,9 +128,12 @@ long getPingPeriod() { * Closes all open connections and disconnects {@link UdpListener}. */ public void close() { - for (Connection connection : connections) { - if (connection != null && connection.isActive()) { - connection.disconnect(); + synchronized (connectionsLock) { + closed = true; + for (Connection connection : connections) { + if (connection != null && connection.isActive()) { + connection.disconnect(); + } } } messenger.closeConnection(); @@ -249,6 +253,9 @@ public int createConnection(InetAddress address, int port, int connectionPingPer private int connect(InetAddress address, int port, long connectionPingPeriod, boolean skipCiphers) throws IOException { synchronized (connectionsLock) { + if (closed) { + throw new IllegalStateException("The connection manager is closed"); + } Connection connection = new Connection(messenger, connections.size()); connection.connect(address, port, connectionPingPeriod, skipCiphers); connections.add(connection); diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index 9095722..818c6a0 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -23,7 +23,6 @@ */ import org.metricshub.ipmi.core.coding.PayloadCoder; -import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder; import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; @@ -70,14 +69,9 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } - // A reply to a message whose reply was not awaited (the keep-alive) may arrive once its sequence number - // has been given to a queued request: the reply of a request answers the command of that request - if (coder instanceof IpmiCommandCoder - && ((IpmiCommandCoder) coder).getCommandCode() != lanMessagePayload.getCommand()) { - LOGGER - .debug( - "Message tagged with " + tag + " answers command " + lanMessagePayload.getCommand() - + ", not the queued request. Dropping stale message."); + if (coder instanceof Connection.KeepAlive) { + // Nobody waits for the reply of the keep-alive: it only frees the tag + getMessageQueue().remove(tag); return; } diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index af3f1bb..d398f5d 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -39,7 +39,7 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world reply is sent again; * the [keep-alive](configuration.html#keep-alive) is actually sent: with the default `pingPeriod` (`-1`) 1.2.02 sent no keep-alive at all, so a session could expire during a long collection; the - keep-alive is now one message every 30 s by default, sent without waiting for its reply, and a + keep-alive is now one message every 30 s by default, whose reply is discarded, and a Get Channel Authentication Capabilities command sent by the application in a session gets its reply (1.2.02 dropped it, as it did the keep-alive replies); * `IpmiConnector.closeConnection()` releases the connection: its handle then throws diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java index 4ac6667..0933792 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionManagerTest.java @@ -136,4 +136,15 @@ void everyOperationOnAReleasedHandleFailsTheSameWay() throws Exception { manager.close(); } } + + @Test + void aClosedManagerCreatesNoConnection() throws Exception { + ConnectionManager manager = new ConnectionManager(new SilentMessenger()); + int handle = manager.createConnection(InetAddress.getLoopbackAddress(), 623); + manager.close(); + assertFalse(manager.getConnection(handle).isActive(), "close() disconnects the connections"); + assertThrows( + IllegalStateException.class, + () -> manager.createConnection(InetAddress.getLoopbackAddress(), 623)); + } } diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index 1ade6ad..2ba3623 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -201,7 +201,7 @@ public void processRequest(IpmiPayload payload) { } @Test - void aReplyToAnotherCommandWithTheTagOfAQueuedRequestIsDropped() throws Exception { + void theKeepAliveReplyIsDiscardedAndFreesItsTag() throws Exception { Connection connection = connect(TIMEOUT_MS); try { openSession(connection); @@ -217,6 +217,13 @@ public void processRequest(IpmiPayload payload) { // not expected } }); + connection.run(); + int keepAliveTag = 1; // the first tag of a new connection + // The keep-alive reply (command 38h) is not delivered, and its tag is free again + connection.notify(new MessageAction(reply(keepAliveTag, (byte) 0x38))); + assertEquals(-1, notifiedTag.get(), "the keep-alive reply must not reach the listeners"); + + // The same command sent by the application gets its reply GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( IpmiVersion.V20, IpmiVersion.V20, @@ -224,12 +231,7 @@ public void processRequest(IpmiPayload payload) { PrivilegeLevel.Callback, (byte) 0xe); int tag = connection.sendMessage(request, false); - - // A late reply to a one-way Get Device ID (command 01h) whose sequence number was reused by the request - connection.notify(new MessageAction(reply(tag, (byte) 0x01))); - assertEquals(-1, notifiedTag.get(), "a reply to another command must not answer the queued request"); - - // The request is still queued: its own reply (command 38h) is delivered + assertEquals(keepAliveTag + 1, tag); connection.notify(new MessageAction(reply(tag, (byte) 0x38))); assertEquals(tag, notifiedTag.get()); } finally { From bd5715a133e9ca9b6f44d2b05b4e0227efa03f40 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:37:37 +0200 Subject: [PATCH 5/8] Register with the messenger outside the registry lock; keep-alive timeouts stay silent ConnectionManager.connect() reserves the slot under connectionsLock and registers with the messenger outside it, then disconnects and throws if close() ran meanwhile; close() flags the manager closed under the lock and disconnects outside it. A listener callback on the receiving thread, which holds the messenger lock, may therefore create or close connections without deadlocking. MessageQueue does not report the timeout of a Connection.KeepAlive, which nobody waits for; the class is public so the queue package can recognize it. The README upgrade summary lists the public changes. Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- .../ipmi/core/connection/Connection.java | 11 +++++--- .../core/connection/ConnectionManager.java | 25 +++++++++++++------ .../core/connection/queue/MessageQueue.java | 5 ++-- .../connection/queue/MessageQueueTest.java | 15 +++++++++++ 5 files changed, 44 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 50530f6..0e10108 100644 --- a/README.md +++ b/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; and `Constants.TIMEOUT`, which nothing reads, is deprecated. ## Build instructions diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index 1a41cda..f36cf12 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java @@ -669,10 +669,15 @@ public void notify(StateMachineAction action) { /** * The keep-alive request: queued like any request, so that its tag stays reserved until its reply arrives or it - * times out, and recognized by {@link IpmiMessageHandler}, which discards its reply. + * times out, but owned by nobody: its reply is discarded and its timeout is not reported to the listeners. */ - static final class KeepAlive extends GetChannelAuthenticationCapabilities { - KeepAlive(CipherSuite cipherSuite) { + 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)); } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index cf39a31..7bd0137 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -45,7 +45,7 @@ public class ConnectionManager { // connect() takes a state machine lock under the list lock private List connections; private final Object connectionsLock = new Object(); - private boolean closed; + private volatile boolean closed; private static final Object SESSIONLESS_TAG_LOCK = new Object(); private static int sessionlessTag; @@ -130,10 +130,12 @@ long getPingPeriod() { public void close() { synchronized (connectionsLock) { closed = true; - for (Connection connection : connections) { - if (connection != null && connection.isActive()) { - connection.disconnect(); - } + } + // Outside the lock: disconnect() unregisters from the messenger, whose receiving thread holds its own lock + // while it notifies the application, which may create or close connections + for (Connection connection : connections) { + if (connection != null && connection.isActive()) { + connection.disconnect(); } } messenger.closeConnection(); @@ -252,15 +254,22 @@ public int createConnection(InetAddress address, int port, int connectionPingPer private int connect(InetAddress address, int port, long connectionPingPeriod, boolean skipCiphers) throws IOException { + Connection connection; synchronized (connectionsLock) { if (closed) { throw new IllegalStateException("The connection manager is closed"); } - Connection connection = new Connection(messenger, connections.size()); - connection.connect(address, port, connectionPingPeriod, skipCiphers); + connection = new Connection(messenger, connections.size()); connections.add(connection); - return connections.size() - 1; } + // Outside the lock: connect() registers with the messenger (see close()) + connection.connect(address, port, connectionPingPeriod, skipCiphers); + if (closed) { + // close() ran meanwhile and may have missed this connection + connection.disconnect(); + throw new IllegalStateException("The connection manager is closed"); + } + return connection.getHandle(); } /** diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java index 74c4335..1d69e3c 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java @@ -353,7 +353,8 @@ private boolean messageJustTimedOut(QueueElement oldestQueueElement) { /** * Removes the oldest message from the queue; when it timed out (rather than being answered), the response - * listeners are told so, which lets the sender retry it with a fresh tag. + * listeners are told so, which lets the sender retry it with a fresh tag. Nobody waits for a keep-alive: its + * timeout is not reported. */ private void processObsoleteMessage(QueueElement message, boolean done) { int tag = message.getId(); @@ -361,7 +362,7 @@ private void processObsoleteMessage(QueueElement message, boolean done) { queue.remove(0); releaseTag(tag); - if (!done) { + if (!done && !(message.getRequest() instanceof Connection.KeepAlive)) { logger.debug("Message timed out, tag: {}", tag); try { connection diff --git a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java index 7d2699e..2dd2ee8 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java @@ -135,4 +135,19 @@ void oneWaySequenceNumbersSkipTheQueuedTags() { queue.tearDown(); } } + + @Test + void aTimedOutKeepAliveIsNotReported() throws Exception { + MessageQueue queue = newQueue(); + try { + connection.registerListener(recorder()); + int keepAlive = queue.add(new Connection.KeepAlive(CipherSuite.getEmpty())); + int request = queue.add(request()); + Thread.sleep(TIMER_TICK_MS); + assertFalse(queue.containsId(keepAlive), "the keep-alive must leave the queue"); + assertEquals(Collections.singletonList(request + ":Message timed out"), reported); + } finally { + queue.tearDown(); + } + } } From 2c68ad9390e4f895ef70743a8124cbd725665622 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:47:54 +0200 Subject: [PATCH 6/8] Drop a reply that answers another command; document Connection.KeepAlive IpmiMessageHandler drops a reply whose command is not the command of the queued request with that tag and leaves the request queued: a late reply to a one-way message, whose tag is not reserved, can carry the tag of a newer request. The low-level API page and the README describe the Connection.KeepAlive request. Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- .../core/connection/IpmiMessageHandler.java | 12 ++++++ src/site/markdown/low-level-api.md | 6 +++ .../ipmi/core/connection/ConnectionTest.java | 37 +++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 0e10108..b99164d 100644 --- a/README.md +++ b/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`. `IpmiConnector.closeConnection()` releases the connection, whose handle then throws `IllegalStateException`; the keep-alive is actually sent with the default configuration; and `Constants.TIMEOUT`, which nothing reads, is deprecated. +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 diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index 818c6a0..734cb29 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -23,6 +23,7 @@ */ import org.metricshub.ipmi.core.coding.PayloadCoder; +import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder; import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; @@ -75,6 +76,17 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } + // A late reply to a one-way message, whose tag is not reserved, may carry the tag of a newer queued + // request: the reply of a request answers the command of that request, and the request stays queued + if (coder instanceof IpmiCommandCoder + && ((IpmiCommandCoder) coder).getCommandCode() != lanMessagePayload.getCommand()) { + LOGGER + .debug( + "Message tagged with " + tag + " answers command " + lanMessagePayload.getCommand() + + ", not the queued request. Dropping stale message."); + return; + } + try { ResponseData responseData = coder.getResponseData(message); getConnection().notifyResponseListeners(getConnection().getHandle(), tag, responseData, null); diff --git a/src/site/markdown/low-level-api.md b/src/site/markdown/low-level-api.md index 9a109ea..acb551d 100644 --- a/src/site/markdown/low-level-api.md +++ b/src/site/markdown/low-level-api.md @@ -91,6 +91,12 @@ port (or always pass `0`), and call `tearDown()` when you are done with it. | --- | --- | | `IpmiConnector(int port)`, `IpmiConnector(int port, InetAddress address)` | Bind the given local port (`0`: any free port), on all interfaces or on one, with the keep-alive period of [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) (30 000 ms). | | `IpmiConnector(int port, long pingPeriod)` | The same, with a [keep-alive period](configuration.html#keep-alive) in ms (`0`: none). | + +The keep-alive is a `Connection.KeepAlive` request, a Get Channel Authentication Capabilities that +the connection queues every `pingPeriod` ms while a session is open. Nobody owns it: its reply is +discarded instead of being delivered to the listeners, and its timeout is not reported. A request +of that class sent by the application gets the same treatment; send a plain +`GetChannelAuthenticationCapabilities` to get the reply. | `createConnection(InetAddress address[, int port])` | Register a connection to a BMC (port 623 by default). | | `createConnection(InetAddress address, [int port,] CipherSuite cipherSuite, PrivilegeLevel level)` | The same, skipping the cipher suite and capabilities steps: call `openSession()` next. | | `closeSession(handle)` | Log out (Close Session). | diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index 2ba3623..3ef0dd9 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -249,4 +249,41 @@ private static Ipmiv20Message reply(int tag, byte command) { reply.setPayload(new IpmiLanResponse(raw)); return reply; } + + @Test + void aReplyToAnotherCommandWithTheTagOfAQueuedRequestIsDropped() throws Exception { + Connection connection = connect(TIMEOUT_MS); + try { + openSession(connection); + AtomicInteger notifiedTag = new AtomicInteger(-1); + connection.registerListener(new ConnectionListener() { + @Override + public void processResponse(ResponseData responseData, int handle, int tag, Exception exception) { + notifiedTag.set(tag); + } + + @Override + public void processRequest(IpmiPayload payload) { + // not expected + } + }); + GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + CipherSuite.getEmpty(), + PrivilegeLevel.Callback, + (byte) 0xe); + int tag = connection.sendMessage(request, false); + + // A late reply to a one-way Get Device ID (command 01h) whose tag was reused by the request + connection.notify(new MessageAction(reply(tag, (byte) 0x01))); + assertEquals(-1, notifiedTag.get(), "a reply to another command must not answer the queued request"); + + // The request is still queued: its own reply (command 38h) is delivered + connection.notify(new MessageAction(reply(tag, (byte) 0x38))); + assertEquals(tag, notifiedTag.get()); + } finally { + connection.disconnect(); + } + } } From c52b0ca5a253104c959394cd2407a57ed8408b2c Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 17:58:50 +0200 Subject: [PATCH 7/8] Match a reply to the queued request by network function as well Command codes are scoped by network function: the reply must carry the command code of the queued request under the response network function of that request (the request one plus one, IPMI 2.0 section 5.1), and a reply whose network function the library does not know answers nothing. Co-Authored-By: Claude Fable 5.1 --- .../core/connection/IpmiMessageHandler.java | 23 +++++++-- .../ipmi/core/connection/ConnectionTest.java | 50 +++++++++++++++++-- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index 734cb29..0383606 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -44,6 +44,22 @@ public IpmiMessageHandler(Connection connection, int timeout) throws IOException super(connection, timeout, IpmiLanMessage.MIN_SEQUENCE_NUMBER, IpmiLanMessage.MAX_SEQUENCE_NUMBER); } + /** + * Tells whether the reply answers the request: the same command code, under the response network function of + * the request (the request one plus one, IPMI 2.0 section 5.1). A reply whose network function the library does + * not know answers nothing. + */ + private static boolean answers(IpmiCommandCoder request, IpmiLanMessage reply) { + if (request.getCommandCode() != reply.getCommand()) { + return false; + } + try { + return request.getNetworkFunction().getCode() + 1 == reply.getNetworkFunction().getCode(); + } catch (IllegalArgumentException e) { + return false; + } + } + /** * If message is of type {@link IpmiLanMessage}, finds corresponding request message and extracts response data from * it, @@ -78,12 +94,11 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { // A late reply to a one-way message, whose tag is not reserved, may carry the tag of a newer queued // request: the reply of a request answers the command of that request, and the request stays queued - if (coder instanceof IpmiCommandCoder - && ((IpmiCommandCoder) coder).getCommandCode() != lanMessagePayload.getCommand()) { + if (coder instanceof IpmiCommandCoder && !answers((IpmiCommandCoder) coder, lanMessagePayload)) { LOGGER .debug( - "Message tagged with " + tag + " answers command " + lanMessagePayload.getCommand() - + ", not the queued request. Dropping stale message."); + "Message tagged with " + tag + " answers another command than the queued request." + + " Dropping stale message."); return; } diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index 3ef0dd9..235a55b 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -185,7 +185,7 @@ public void processRequest(IpmiPayload payload) { assertTrue(tag > 0, "the request must be queued"); // A minimal response carrying the tag of the request (rqSeq, bits 7:2 of byte 4) - byte[] raw = { 0x20, 0x18, 0, (byte) 0x81, (byte) (tag << 2), 0x38, 0, 0 }; + byte[] raw = { 0x20, 0x1c, 0, (byte) 0x81, (byte) (tag << 2), 0x38, 0, 0 }; raw[2] = (byte) -(raw[0] + raw[1]); raw[7] = (byte) -(raw[3] + raw[4] + raw[5] + raw[6]); Ipmiv20Message reply = new Ipmiv20Message(null); @@ -239,9 +239,14 @@ public void processRequest(IpmiPayload payload) { } } - /** A minimal IPMI LAN response with the given tag (rqSeq, bits 7:2 of byte 4) and command. */ + /** A minimal Application (07h) IPMI LAN response with the given tag (rqSeq, bits 7:2 of byte 4) and command. */ private static Ipmiv20Message reply(int tag, byte command) { - byte[] raw = { 0x20, 0x18, 0, (byte) 0x81, (byte) (tag << 2), command, 0, 0 }; + return reply(tag, (byte) 0x07, command); + } + + /** A minimal IPMI LAN response with the given tag, response network function and command. */ + private static Ipmiv20Message reply(int tag, byte networkFunction, byte command) { + byte[] raw = { 0x20, (byte) (networkFunction << 2), 0, (byte) 0x81, (byte) (tag << 2), command, 0, 0 }; raw[2] = (byte) -(raw[0] + raw[1]); raw[7] = (byte) -(raw[3] + raw[4] + raw[5] + raw[6]); Ipmiv20Message reply = new Ipmiv20Message(null); @@ -286,4 +291,43 @@ public void processRequest(IpmiPayload payload) { connection.disconnect(); } } + + @Test + void aReplyUnderAnotherNetworkFunctionWithTheTagOfAQueuedRequestIsDropped() throws Exception { + Connection connection = connect(TIMEOUT_MS); + try { + openSession(connection); + AtomicInteger notifiedTag = new AtomicInteger(-1); + connection.registerListener(new ConnectionListener() { + @Override + public void processResponse(ResponseData responseData, int handle, int tag, Exception exception) { + notifiedTag.set(tag); + } + + @Override + public void processRequest(IpmiPayload payload) { + // not expected + } + }); + GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + CipherSuite.getEmpty(), + PrivilegeLevel.Callback, + (byte) 0xe); + int tag = connection.sendMessage(request, false); + + // Command 38h under the Chassis response network function (01h): not the Application 38h queued + connection.notify(new MessageAction(reply(tag, (byte) 0x01, (byte) 0x38))); + assertEquals(-1, notifiedTag.get(), "a reply under another network function must not answer the request"); + // An unknown network function (0Fh) answers nothing either + connection.notify(new MessageAction(reply(tag, (byte) 0x0f, (byte) 0x38))); + assertEquals(-1, notifiedTag.get()); + + connection.notify(new MessageAction(reply(tag, (byte) 0x38))); + assertEquals(tag, notifiedTag.get(), "the request is still queued and gets its own reply"); + } finally { + connection.disconnect(); + } + } } From 7f032dcdd463964dd2b2bf2f8293a395c1bed5d7 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 18:10:19 +0200 Subject: [PATCH 8/8] Move the keep-alive paragraph after the connector method table The paragraph ended the Markdown table before its last rows. Co-Authored-By: Claude Fable 5.1 --- src/site/markdown/low-level-api.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/site/markdown/low-level-api.md b/src/site/markdown/low-level-api.md index acb551d..277e9b6 100644 --- a/src/site/markdown/low-level-api.md +++ b/src/site/markdown/low-level-api.md @@ -91,17 +91,17 @@ port (or always pass `0`), and call `tearDown()` when you are done with it. | --- | --- | | `IpmiConnector(int port)`, `IpmiConnector(int port, InetAddress address)` | Bind the given local port (`0`: any free port), on all interfaces or on one, with the keep-alive period of [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) (30 000 ms). | | `IpmiConnector(int port, long pingPeriod)` | The same, with a [keep-alive period](configuration.html#keep-alive) in ms (`0`: none). | +| `createConnection(InetAddress address[, int port])` | Register a connection to a BMC (port 623 by default). | +| `createConnection(InetAddress address, [int port,] CipherSuite cipherSuite, PrivilegeLevel level)` | The same, skipping the cipher suite and capabilities steps: call `openSession()` next. | +| `closeSession(handle)` | Log out (Close Session). | +| `closeConnection(handle)` | Close and release the connection; the handle is no longer usable. | +| `tearDown()` | Close every connection and release the local port. | The keep-alive is a `Connection.KeepAlive` request, a Get Channel Authentication Capabilities that the connection queues every `pingPeriod` ms while a session is open. Nobody owns it: its reply is discarded instead of being delivered to the listeners, and its timeout is not reported. A request of that class sent by the application gets the same treatment; send a plain `GetChannelAuthenticationCapabilities` to get the reply. -| `createConnection(InetAddress address[, int port])` | Register a connection to a BMC (port 623 by default). | -| `createConnection(InetAddress address, [int port,] CipherSuite cipherSuite, PrivilegeLevel level)` | The same, skipping the cipher suite and capabilities steps: call `openSession()` next. | -| `closeSession(handle)` | Log out (Close Session). | -| `closeConnection(handle)` | Close and release the connection; the handle is no longer usable. | -| `tearDown()` | Close every connection and release the local port. | ### Choosing the cipher suite