Fail a bad login at once and clean up inside the overall timeout (#109, #132)

src/main/java/org/metricshub/ipmi/client/IpmiClient.java

@@ -55,9 +55,7 @@ public static GetChassisStatusResponseData getChassisStatus(final IpmiClientConf
 			throws InterruptedException,
 			ExecutionException,
 			TimeoutException {
-		try (GetChassisStatusRunner runner = new GetChassisStatusRunner(ipmiConfiguration)) {
-			return execute(runner, ipmiConfiguration.getTimeout() * 1000);
-		}
+		return execute(new GetChassisStatusRunner(ipmiConfiguration), ipmiConfiguration.getTimeout() * 1000);
 	}
 
 	/**
@@ -73,9 +71,7 @@ public static List<Sensor> getSensors(final IpmiClientConfiguration ipmiConfigur
 			throws InterruptedException,
 			ExecutionException,
 			TimeoutException {
-		try (GetSensorsRunner runner = new GetSensorsRunner(ipmiConfiguration)) {
-			return execute(runner, ipmiConfiguration.getTimeout() * 1000);
-		}
+		return execute(new GetSensorsRunner(ipmiConfiguration), ipmiConfiguration.getTimeout() * 1000);
 	}
 
 	/**
@@ -91,9 +87,7 @@ public static List<Fru> getFrus(final IpmiClientConfiguration ipmiConfiguration)
 			throws InterruptedException,
 			ExecutionException,
 			TimeoutException {
-		try (GetFrusRunner runner = new GetFrusRunner(ipmiConfiguration)) {
-			return execute(runner, ipmiConfiguration.getTimeout() * 1000);
-		}
+		return execute(new GetFrusRunner(ipmiConfiguration), ipmiConfiguration.getTimeout() * 1000);
 	}
 
 	/**

src/main/java/org/metricshub/ipmi/client/Utils.java

@@ -31,13 +31,22 @@
 import java.util.concurrent.TimeoutException;
 
 import org.metricshub.ipmi.client.runner.AbstractIpmiRunner;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public final class Utils {
 
+	private static final Logger LOGGER = LoggerFactory.getLogger(Utils.class);
+
 	private Utils() {}
 
 	public static final String EMPTY = "";
 
+	/**
+	 * How long a call that hit its deadline waits for its worker to close the session and the socket.
+	 */
+	private static final long CLEANUP_GRACE_MS = 1000;
+
 	/**
 	 * @param value The value to check
 	 * @return whether the value is null, empty or contains only blank chars
@@ -87,19 +96,50 @@ public static <T> T execute(final AbstractIpmiRunner<T> callable, long timeout)
 			ExecutionException,
 			TimeoutException {
 
-		final ExecutorService executorService = Executors.newSingleThreadExecutor();
-		final Future<T> future = executorService.submit(callable);
+		final ExecutorService executorService = Executors.newSingleThreadExecutor(Utils::newWorkerThread);
+
+		// The worker owns the connector: it also closes it, so the cleanup is covered by the deadline and never
+		// runs on the calling thread while the worker is still using the connection
+		final Future<T> future = executorService.submit(() -> {
+			try (AbstractIpmiRunner<T> runner = callable) {
+				return runner.call();
+			}
+		});
 
 		try {
 			return future.get(timeout, TimeUnit.MILLISECONDS);
 		} catch (InterruptedException e) {
+			stopWorker(future, executorService);
 			Thread.currentThread().interrupt();
 			throw e;
 		} catch (TimeoutException e) {
-			future.cancel(true);
+			stopWorker(future, executorService);
 			throw e;
 		} finally {
 			executorService.shutdownNow();
 		}
 	}
+
+	/**
+	 * Stops the worker at its current wait and gives it a moment to close the session and release the port.
+	 */
+	private static void stopWorker(Future<?> future, ExecutorService executorService) {
+		future.cancel(true);
+		executorService.shutdownNow();
+		try {
+			if (!executorService.awaitTermination(CLEANUP_GRACE_MS, TimeUnit.MILLISECONDS)) {
+				// A call that cannot be interrupted (name resolution, a blocking send): the worker closes the
+				// connection and releases the port by itself when that call returns
+				LOGGER.warn("The IPMI worker is still busy after the call was abandoned; the port is released when it returns");
+			}
+		} catch (InterruptedException e) {
+			Thread.currentThread().interrupt();
+		}
+	}
+
+	private static Thread newWorkerThread(Runnable runnable) {
+		Thread thread = new Thread(runnable, "ipmi-client");
+		thread.setDaemon(true);
+		return thread;
+	}
 }

src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java

@@ -243,6 +243,7 @@ public void close() {
 
 		// Close connection manager and release the listener port.
 		connector.tearDown();
+		connector = null;
 	}
 
 	/**

src/main/java/org/metricshub/ipmi/core/api/async/IpmiAsyncConnector.java

@@ -30,6 +30,7 @@
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
 import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilitiesResponseData;
 import org.metricshub.ipmi.core.coding.payload.IpmiPayload;
+import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.protocol.PayloadType;
 import org.metricshub.ipmi.core.coding.security.CipherSuite;
 import org.metricshub.ipmi.core.common.PropertiesManager;
@@ -226,13 +227,11 @@ public List<CipherSuite> getAvailableCipherSuites(
 				++tries;
 				result = connectionManager
 						.getAvailableCipherSuites(connectionHandle.getHandle());
-			} catch (InterruptedException e) {
-				throw e;
 			} catch (Exception e) {
-				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
-				if (tries > retries) {
+				if (tries > retries || !isRetriable(e)) {
 					throw e;
 				}
+				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
 			}
 		}
 		return result;
@@ -272,13 +271,11 @@ public GetChannelAuthenticationCapabilitiesResponseData getChannelAuthentication
 								requestedPrivilegeLevel);
 				connectionHandle.setCipherSuite(cipherSuite);
 				connectionHandle.setPrivilegeLevel(requestedPrivilegeLevel);
-			} catch (InterruptedException e) {
-				throw e;
 			} catch (Exception e) {
-				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
-				if (tries > retries) {
+				if (tries > retries || !isRetriable(e)) {
 					throw e;
 				}
+				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
 			}
 		}
 		return result;
@@ -331,13 +328,11 @@ public Session openSession(
 				session = sessionManager.registerSession(sessionId, connectionHandle);
 
 				succeded = true;
-			} catch (InterruptedException e) {
-				throw e;
 			} catch (Exception e) {
-				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
-				if (tries > retries) {
+				if (tries > retries || !isRetriable(e)) {
 					throw e;
 				}
+				logger.warn(FAILED_TO_RECEIVE_ANSWER_CAUSE_MESSAGE, e);
 			}
 		}
 
@@ -602,4 +597,14 @@ public int getTimeout(ConnectionHandle handle) {
 		return connectionManager.getConnection(handle.getHandle()).getTimeout();
 	}
 
+	/**
+	 * Tells whether a failed handshake step is worth sending again: when no reply came, or when the BMC answered
+	 * with a transient completion code. Any other answer (wrong credentials, unknown user, refused cipher suite...)
+	 * would be the same the next time, and resending the credentials trips account lockouts.
+	 */
+	private static boolean isRetriable(Exception e) {
+		return e instanceof ConnectionException
+				|| (e instanceof IPMIException && ((IPMIException) e).getCompletionCode().isTransient());
+	}
+
 }

src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java

@@ -29,7 +29,6 @@
 import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel;
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
 import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilitiesResponseData;
-import org.metricshub.ipmi.core.coding.payload.CompletionCode;
 import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.protocol.PayloadType;
 import org.metricshub.ipmi.core.coding.security.CipherSuite;
@@ -449,11 +448,7 @@ private void handleRetriesWhenException(int tries, Exception e) throws Exception
 	}
 
 	private void handleErrorResponse(int tries, IPMIException e) throws Exception {
-		if (e.getCompletionCode() == CompletionCode.InitializationInProgress
-				|| e.getCompletionCode() == CompletionCode.InsufficientResources
-				|| e.getCompletionCode() == CompletionCode.NodeBusy
-				|| e.getCompletionCode() == CompletionCode.Timeout) {
-
+		if (e.getCompletionCode().isTransient()) {
 			handleRetriesWhenException(tries, e);
 		} else {
 			throw e;

src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java

@@ -276,6 +276,16 @@ public int getCode() {
 		return code;
 	}
 
+	/**
+	 * Tells whether the BMC may answer the same request successfully a little later.
+	 *
+	 * @return true for the completion codes that only report a passing condition of the BMC (busy, out of resources,
+	 *         initializing, internal timeout), false for every other code
+	 */
+	public boolean isTransient() {
+		return this == InitializationInProgress || this == InsufficientResources || this == NodeBusy || this == Timeout;
+	}
+
 	public static CompletionCode parseInt(int value) {
 		switch (value) {
 		case OK:

src/main/java/org/metricshub/ipmi/core/sm/states/AuthcapWaiting.java

@@ -98,6 +98,7 @@ public void doAction(StateMachine stateMachine, RmcpMessage message) {
 												.getResponseData(ipmiMessage)));
 			}
 		} catch (Exception e) {
+			stateMachine.setCurrent(new Ciphers());
 			stateMachine.doExternalAction(new ErrorAction(e));
 		}
 	}

src/main/java/org/metricshub/ipmi/core/sm/states/CiphersWaiting.java

@@ -91,6 +91,7 @@ public void doTransition(
 												0));
 				++index;
 			} catch (Exception e) {
+				stateMachine.setCurrent(new Uninitialized());
 				stateMachine.doExternalAction(new ErrorAction(e));
 			}
 		} else if (machineEvent instanceof DefaultAck) {
@@ -135,6 +136,7 @@ public void doAction(StateMachine stateMachine, RmcpMessage message) {
 												.getResponseData(ipmiMessage)));
 			}
 		} catch (Exception e) {
+			stateMachine.setCurrent(new Uninitialized());
 			stateMachine.doExternalAction(new ErrorAction(e));
 		}
 	}

src/main/java/org/metricshub/ipmi/core/sm/states/OpenSessionWaiting.java

@@ -97,6 +97,7 @@ public void doAction(StateMachine stateMachine, RmcpMessage message) {
 												.getResponseData(ipmiMessage)));
 			}
 		} catch (Exception e) {
+			stateMachine.setCurrent(new Authcap());
 			stateMachine.doExternalAction(new ErrorAction(e));
 		}
 	}

src/main/java/org/metricshub/ipmi/core/sm/states/Rakp1Waiting.java

@@ -107,6 +107,7 @@ public void doAction(StateMachine stateMachine, RmcpMessage message) {
 												.getResponseData(ipmiMessage)));
 			}
 		} catch (Exception e) {
+			stateMachine.setCurrent(new Authcap());
 			stateMachine.doExternalAction(new ErrorAction(e));
 		}
 	}

src/main/java/org/metricshub/ipmi/core/sm/states/Rakp3Waiting.java

@@ -121,6 +121,7 @@ public void doAction(StateMachine stateMachine, RmcpMessage message) {
 												.getResponseData(ipmiMessage)));
 			}
 		} catch (Exception e) {
+			stateMachine.setCurrent(new Authcap());
 			stateMachine.doExternalAction(new ErrorAction(e));
 		}
 	}

src/site/markdown/timeouts-and-errors.md

@@ -21,8 +21,11 @@ the session — in a worker thread, and waits for it at most `timeout` seconds.
 expires, the worker is interrupted and the method throws `java.util.concurrent.TimeoutException`,
 with nothing collected: there are no partial results.
 
-The interrupted worker stops at its current wait, and the library's receiving and timer threads
-are daemon threads: they never keep the JVM alive.
+The interrupted worker stops at its current wait, closes the session and releases the UDP port,
+and the method waits up to one second for that cleanup before throwing. A worker stuck in a call
+that cannot be interrupted (name resolution, for example) closes the connection when that call
+returns; a `WARN` says so. The library's receiving and timer threads are daemon threads: they
+never keep the JVM alive.
 
 ## Per-message timeout and retries
 
@@ -91,7 +94,8 @@ Common causes wrapped in the `ExecutionException`:
 
 | Cause | Meaning |
 | --- | --- |
-| `ConnectionException: Illegal connection state: Rakp1Waiting` | The RAKP handshake failed: wrong user name or password, account not allowed over LAN or at the User level. The `ERROR` log shows the actual reason (`Authentication check failed`, ...), see [#109](https://github.com/metricshub/ipmi-java/issues/109). |
+| `IllegalArgumentException: Authentication check failed` | The RAKP handshake failed: the BMC's proof does not match the password or the [BMC key](configuration.html#bmc-key). The credentials are sent once. |
+| `IPMIException: Unauthorized name.`, `Invalid role.`, ... | The BMC refused the session: unknown user, user not allowed over the LAN channel or at the User level, cipher suite refused ([Troubleshooting](troubleshooting.html#the-login-fails)). |
 | `ConnectionException: Command timed out` / `Message timed out` | No reply after all the [tries](#per-message-timeout-and-retries) of a message: `Command timed out` during the session handshake, the usual symptom of a wrong host, a closed UDP port or IPMI over LAN disabled; `Message timed out` in the session. |
 | `IPMIException` | The BMC answered with an error completion code. `getCompletionCode()` returns it, for example `InsufficientPrivilege` (`0xD4`). |
 | `IllegalArgumentException: ... is not yet implemented.` | The chosen cipher suite uses an algorithm the client does not implement (xRC4, MD5-128). See [cipher suites](preparing-the-bmc.html#cipher-suites). |

src/site/markdown/troubleshooting.md

@@ -1,5 +1,5 @@
-keywords: troubleshooting, timeout, illegal connection state, rakp1waiting, authentication check failed, insufficient privilege, 0xd4, ipmitool, ipmiutil, debug
-description: Diagnose the usual failures of the IPMI Java Client — timeouts, authentication errors, insufficient privilege, missing sensors or FRUs, a JVM that does not exit — and compare with ipmitool and ipmiutil.
+keywords: troubleshooting, timeout, command timed out, login, authentication check failed, unauthorized name, insufficient privilege, 0xd4, ipmitool, ipmiutil, debug
+description: Diagnose the usual failures of the IPMI Java Client — timeouts, authentication errors, insufficient privilege, missing sensors or FRUs — and compare with ipmitool and ipmiutil.
 
 # Troubleshooting
 
@@ -37,14 +37,12 @@ The stack trace of the `Command timed out` shows the step that was waiting. A fa
 `getAvailableCipherSuites` means the BMC never answered the very first request: the address, the
 port or the firewall is wrong, or IPMI over LAN is disabled.
 
-## `Illegal connection state: Rakp1Waiting`
+## The login fails
 
-The `ExecutionException` wraps
-`ConnectionException: Illegal connection state: Rakp1Waiting`: the RAKP handshake (the login)
-failed, and the session could not be opened. The actual reason is logged just before, at the
-`ERROR` level ([#109](https://github.com/metricshub/ipmi-java/issues/109)):
+The RAKP handshake (the login) failed and the session could not be opened. The credentials are
+sent once; the `ExecutionException` wraps the reason:
 
-| Logged | Cause |
+| Cause | Meaning |
 | --- | --- |
 | `IllegalArgumentException: Authentication check failed` | The BMC's proof does not match the password: **wrong password** (or wrong [BMC key](configuration.html#bmc-key)). |
 | `IPMIException: Unauthorized name.` | **Unknown user**, or a user not allowed to log in over the LAN channel. |
@@ -84,12 +82,6 @@ to make it use suite 3 or 17.
 | A sensor reads `0.0` | The BMC flags the reading as unavailable, which is not checked yet ([#110](https://github.com/metricshub/ipmi-java/issues/110)). |
 | Negative processor temperatures (`CPU1 DTS = -44.0`) | Not an error: Intel *Digital Thermal Sensor* readings are the margin below the maximum junction temperature. |
 
-## The JVM does not exit
-
-After a `TimeoutException`, some threads of the library may still run, and they are not daemon
-threads ([#79](https://github.com/metricshub/ipmi-java/issues/79)). End command-line programs and
-test harnesses with `System.exit(0)`.
-
 ## Collecting is slow
 
 * **FRUs**: each FRU is read 16 bytes at a time, one round trip per chunk: a few seconds per FRU

src/site/markdown/upgrading.md

@@ -28,8 +28,15 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world
   `ExecutionException` wrapping `ConnectionException: Command timed out`, about 20 s into the
   call, where 1.2.02 threw `TimeoutException` at the overall timeout;
 * the overall timeout cancels the worker for good: the interrupted session stops at its current
-  wait, and the receiving and timer threads are daemon threads, so a program no longer needs
-  `System.exit()` to end.
+  wait and closes the session and the port, normally within one second of the deadline (a worker
+  stuck in a call that cannot be interrupted, such as name resolution, does so when that call
+  returns), and the receiving and timer threads are daemon threads, so a program no longer needs
+  `System.exit()` to end;
+* a failed login fails at once with its actual cause (`IllegalArgumentException: Authentication
+  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.
 
 Code that **extends** the library's protocol classes needs the changes below. `QueueElement`
 lost its `isTimedOut()`, `makeTimedOut()` and `refreshTimestamp()` methods: a timed-out message