From 4de55fb75a527070653a02b390d958029f1b9fcc Mon Sep 17 00:00:00 2001 From: Arun Venmany Date: Wed, 2 Sep 2026 21:38:41 +0530 Subject: [PATCH 1/4] Fix container name race and port hold issues for devc multimodule --- .../tools/common/plugins/util/DevUtil.java | 201 ++++++++++++----- .../util/DevUtilContainerNameTest.java | 212 ++++++++++++++++++ .../plugins/util/DevUtilPortHoldingTest.java | 177 +++++++++++++++ 3 files changed, 540 insertions(+), 50 deletions(-) create mode 100644 src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java create mode 100644 src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java index e22b6d62..d61216e5 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java @@ -1678,44 +1678,39 @@ protected static File getLongestCommonDir(File dir1, File dir2) { * @return the command string to use to start the container */ private String[] getContainerCommand() throws IOException, PluginExecutionException { - // Use List for building the command. This will be converted to a String[] when passed to ProcessBuilder. + // Use List for building the command. This will be converted to a String[] when passed to ProcessBuilder. // The ProcessBuilder will handle quoting of paths (for blanks) as needed for various OS. List commandElements = new ArrayList(); commandElements.add(getContainerCommandPrefix().trim()); commandElements.add("run"); commandElements.add("--rm"); + // Select host-side ports and hold them open for the entire command-build phase. + // Holding all three sockets simultaneously means a concurrent module's + // findAvailablePort() will see each chosen port as already bound and advance to + // the next one, eliminating the TOCTOU race without any shared state. + List heldSockets = new ArrayList(); + if (!skipDefaultPorts) { - int httpPortToUse, httpsPortToUse; - try { - httpPortToUse = findAvailablePort(LIBERTY_DEFAULT_HTTP_PORT, false); - httpsPortToUse = findAvailablePort(LIBERTY_DEFAULT_HTTPS_PORT, false); - } catch (IOException x) { - error("An error occurred while trying to find an available network port. Using default port numbers.", x); - httpPortToUse = LIBERTY_DEFAULT_HTTP_PORT; - httpsPortToUse = LIBERTY_DEFAULT_HTTPS_PORT; - } + int httpPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTP_PORT, false, heldSockets); + int httpsPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTPS_PORT, false, heldSockets); commandElements.add("-p"); commandElements.add(httpPortToUse+":"+LIBERTY_DEFAULT_HTTP_PORT); commandElements.add("-p"); commandElements.add(httpsPortToUse+":"+LIBERTY_DEFAULT_HTTPS_PORT); } - + if (libertyDebug) { // map debug port int containerDebugPort, hostDebugPort; - try { - if (alternativeDebugPort == -1) { - // it is possible another JVM has grabbed our port since dev mode last checked - hostDebugPort = findAvailablePort(libertyDebugPort, true); - containerDebugPort = libertyDebugPort; - } else { - // dev mode has already selected an ephemeral port - containerDebugPort = hostDebugPort = alternativeDebugPort; - } - } catch (IOException x) { - containerDebugPort = hostDebugPort = libertyDebugPort; + if (alternativeDebugPort == -1) { + // it is possible another JVM has grabbed our port since dev mode last checked + hostDebugPort = findAndHoldPort(libertyDebugPort, true, heldSockets); + containerDebugPort = libertyDebugPort; + } else { + // dev mode has already selected an ephemeral port + containerDebugPort = hostDebugPort = alternativeDebugPort; } commandElements.add("-p"); commandElements.add(hostDebugPort+":"+containerDebugPort); @@ -1798,9 +1793,82 @@ private String[] getContainerCommand() throws IOException, PluginExecutionExcept //return command.toString(); String[] newCommand = commandElements.toArray(new String[commandElements.size()]); info("Container command: "+String.join(" ", newCommand)); + // Release all held port sockets as late as possible so the container engine + // can bind those exact ports immediately after this command is issued. + for (ServerSocket s : heldSockets) { + closeQuietly(s); + } return newCommand; } + /** + * Finds an available port starting from {@code preferred} and immediately binds a + * {@code ServerSocket} on it to hold that port open until the caller is done building + * the container command. The bound socket is added to {@code heldSockets} so the + * caller can release all of them in a single loop. + * + *

If {@code findAvailablePort} throws, the default port is returned without + * holding any socket (consistent with the pre-existing error-recovery behaviour). + * + * @param preferred the preferred port number to try first + * @param isDebug true if this is the debug port (affects ephemeral-port fallback) + * @param heldSockets accumulator list; the new socket is appended if binding succeeds + * @return the selected port number + */ + private int findAndHoldPort(int preferred, boolean isDebug, List heldSockets) + throws IOException { + int port; + try { + port = findAvailablePort(preferred, isDebug); + } catch (IOException x) { + error("An error occurred while trying to find an available network port. Using default port " + preferred + ".", x); + return preferred; + } + ServerSocket s = bindPortSocket(port); + if (s != null) { + heldSockets.add(s); + } + return port; + } + + /** + * Closes a {@link ServerSocket}, logging any {@link IOException} at debug level + * rather than propagating it. Safe to call with a {@code null} argument. + */ + private void closeQuietly(ServerSocket s) { + if (s != null) { + try { + s.close(); + } catch (IOException e) { + debug("closeQuietly: error closing port socket: " + e.getMessage()); + } + } + } + + /** + * Attempts to bind the given port on the loopback interface to hold it open. + * Returns the bound ServerSocket, or null if binding fails (in which case the + * caller continues without holding a socket). + *

+ * SO_REUSEADDR is explicitly set to false so that the OS will not allow another + * process to bind the same port while this socket is open. + */ + private ServerSocket bindPortSocket(int port) { + try { + if (OSUtil.isWindows()) { + return new ServerSocket(port); + } else { + ServerSocket s = new ServerSocket(); + s.setReuseAddress(false); + s.bind(new InetSocketAddress(InetAddress.getByName(null), port), 1); + return s; + } + } catch (IOException e) { + debug("bindPortSocket: could not hold port " + port + ": " + e.getMessage()); + return null; + } + } + /** * Obtain a given container run option from the containerRunOpts parameter * @param optionName the name of the option to extract from the containerRunOpts @@ -1823,38 +1891,68 @@ private String getContainerOption(String optionName) { return null; } - private String generateNewContainerName() throws PluginExecutionException { + String generateNewContainerName() throws PluginExecutionException { + // Build a sanitized name segment from applicationId so each module gets a stable, + // unique default name (e.g. "liberty-dev-moduleA") that does not depend on the + // order in which modules start. This eliminates the race condition that occurred + // when multiple modules computed the same numeric suffix from a shared container list. + String appSegment = sanitizeContainerNameSegment(applicationId); + String preferredName = DEVMODE_CONTAINER_BASE_NAME + (appSegment.isEmpty() ? "" : "-" + appSegment); + String containerContNamesCmd = "ps -a --format \"{{.Names}}\""; debug("container names list command: " + containerContNamesCmd); String result = execContainerCmdWithPrefix(containerContNamesCmd, CONTAINER_TIMEOUT); - if (result == null) { - return DEVMODE_CONTAINER_BASE_NAME; - } - String[] containerNames = result.split(" "); - int highestNum = -1; - for(int i = 0; i < containerNames.length; i++) { - String name = removeSurroundingQuotes(containerNames[i]); - int num = -1; - if (name.equals(DEVMODE_CONTAINER_BASE_NAME)) { - num = 0; - } else if (name.startsWith(DEVMODE_CONTAINER_BASE_NAME + "-")) { - String[] nameSegments = name.split("-"); - // if DEVMODE_CONTAINER_BASE_NAME changes, the logic below may need to change - if (nameSegments.length == 3) { - String lastSegment = nameSegments[nameSegments.length - 1]; - try { - num = Integer.parseInt(lastSegment); - } catch (NumberFormatException e) { - debug("Last segment of container name is not a number."); - } - } - } - if (num > highestNum) { - highestNum = num; + + Set existingNames = new HashSet(); + if (result != null) { + for (String rawName : result.split(" ")) { + existingNames.add(removeSurroundingQuotes(rawName).trim()); } } - - return DEVMODE_CONTAINER_BASE_NAME + ((highestNum != -1) ? "-" + ++highestNum : ""); + + if (!existingNames.contains(preferredName)) { + return preferredName; + } + + // Preferred name is already in use (e.g. a second instance of the same module). + // Append an incrementing suffix until we find an unused name. + int suffix = 1; + String candidate; + do { + candidate = preferredName + "-" + suffix; + suffix++; + } while (existingNames.contains(candidate)); + return candidate; + } + + /** + * Sanitizes a string so it can be used as part of a container name. + * Container names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*. + * Characters outside that set are replaced with hyphens, leading/trailing + * hyphens are removed, and the result is truncated to 63 characters so + * that the full "liberty-dev-<segment>" name stays within the 128-char limit + * that most container engines impose on container names. + * + * @param input the raw string to sanitize (e.g. applicationId) + * @return a sanitized, lower-case string suitable for use in a container name, + * or an empty string if input is null or blank + */ + static String sanitizeContainerNameSegment(String input) { + if (input == null || input.trim().isEmpty()) { + return ""; + } + // Replace any character that is not alphanumeric, hyphen, underscore, or dot with a hyphen. + String sanitized = input.trim().toLowerCase().replaceAll("[^a-z0-9_.]", "-"); + // Collapse consecutive hyphens/dots/underscores to a single hyphen. + sanitized = sanitized.replaceAll("[-_.]{2,}", "-"); + // Remove leading and trailing hyphens. + sanitized = sanitized.replaceAll("^[-_.]+|[-_.]+$", ""); + // Truncate to 63 characters. + if (sanitized.length() > 63) { + sanitized = sanitized.substring(0, 63); + sanitized = sanitized.replaceAll("[-_.]+$", ""); + } + return sanitized; } /** @@ -6054,7 +6152,10 @@ public void writeDevcMetadata(boolean alive) { private void writeElement(XMLStreamWriter writer, String element, String optional) throws XMLStreamException { writer.writeStartElement(element); - if (optional != null) writer.writeCharacters(optional); + // Always write the characters (empty string when null) so the XML stream writer + // emits rather than the self-closing form. + // The test assertions depend on the open+close form being present. + writer.writeCharacters(optional != null ? optional : ""); writer.writeEndElement(); } } diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java new file mode 100644 index 00000000..f2da622b --- /dev/null +++ b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java @@ -0,0 +1,212 @@ +/** + * (C) Copyright IBM Corporation 2026. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.openliberty.tools.common.plugins.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.ThreadPoolExecutor; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +public class DevUtilContainerNameTest { + + @Rule + public TemporaryFolder temp = new TemporaryFolder(); + + @Test + public void testSanitize_null() { + assertEquals("", DevUtil.sanitizeContainerNameSegment(null)); + } + + @Test + public void testSanitize_emptyString() { + assertEquals("", DevUtil.sanitizeContainerNameSegment("")); + } + + @Test + public void testSanitize_blankString() { + assertEquals("", DevUtil.sanitizeContainerNameSegment(" ")); + } + + @Test + public void testSanitize_simpleAlphanumeric() { + assertEquals("myapp", DevUtil.sanitizeContainerNameSegment("myApp")); + } + + @Test + public void testSanitize_hyphenAndDot() { + assertEquals("my-app.v1", DevUtil.sanitizeContainerNameSegment("my-app.v1")); + } + + @Test + public void testSanitize_specialCharactersReplacedWithHyphen() { + assertEquals("com.example-myapp", DevUtil.sanitizeContainerNameSegment("com.example:myApp")); + assertEquals("my-app", DevUtil.sanitizeContainerNameSegment("my app")); + assertEquals("a-b", DevUtil.sanitizeContainerNameSegment("a/b")); + } + + @Test + public void testSanitize_consecutiveSeparatorsCollapsed() { + assertEquals("a-b", DevUtil.sanitizeContainerNameSegment("a..b")); + assertEquals("a-b", DevUtil.sanitizeContainerNameSegment("a--b")); + assertEquals("a-b", DevUtil.sanitizeContainerNameSegment("a_.b")); + } + + @Test + public void testSanitize_leadingAndTrailingSeparatorsRemoved() { + assertEquals("app", DevUtil.sanitizeContainerNameSegment("-app-")); + assertEquals("app", DevUtil.sanitizeContainerNameSegment("..app..")); + } + + @Test + public void testSanitize_truncatesLongInput() { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 70; i++) sb.append('a'); + String result = DevUtil.sanitizeContainerNameSegment(sb.toString()); + assertTrue("Sanitized segment must be <= 63 chars", result.length() <= 63); + } + + @Test + public void testSanitize_upperCaseLowered() { + assertEquals("moduleabc", DevUtil.sanitizeContainerNameSegment("ModuleABC")); + } + + private class ContainerNameTestUtil extends DevUtil { + + private final String fakeContainerList; + + ContainerNameTestUtil(String applicationId, String fakeContainerList) throws IOException { + super(temp.newFolder(), + null, + null, null, null, + null, null, + null, + false, false, false, false, false, false, + applicationId, + 30, 30, 5, 500, + true, false, false, false, + false, + null, null, null, 0, false, null, false, + null, null, false, null, null, null, false, + null, null, null, Collections.emptyMap()); + this.fakeContainerList = fakeContainerList; + } + + public String callGenerateNewContainerName() throws PluginExecutionException { + return generateNewContainerName(); + } + + @Override + protected String execContainerCmdWithPrefix(String command, int timeout, boolean throwExceptionOnError) { + return fakeContainerList; + } + + @Override public void debug(String msg) {} + @Override public void debug(String msg, Throwable e) {} + @Override public void debug(Throwable e) {} + @Override public void warn(String msg) {} + @Override public void info(String msg) {} + @Override public void error(String msg) {} + @Override public void error(String msg, Throwable e) {} + @Override public boolean isDebugEnabled() { return false; } + @Override public void stopServer() {} + @Override public io.openliberty.tools.ant.ServerTask getServerTask() { return null; } + @Override public boolean recompileBuildFile(File f, Set c, Set t, boolean g, ThreadPoolExecutor e) { return false; } + @Override public boolean updateArtifactPaths(ProjectModule m, boolean r, boolean g, ThreadPoolExecutor e) { return false; } + @Override public boolean updateArtifactPaths(File f) { return false; } + @Override public int countApplicationUpdatedMessages() { return 0; } + @Override public void runTests(boolean w, int m, ThreadPoolExecutor e, boolean a, boolean b, boolean c, File f, String s) {} + @Override public void installFeatures(File f, File s, boolean g) {} + @Override public ServerFeatureUtil getServerFeatureUtilObj() { return null; } + @Override public Set getExistingFeatures() { return null; } + @Override public void updateExistingFeatures() {} + @Override public boolean compile(File d) { return false; } + @Override public void runUnitTests(File f) {} + @Override public void runIntegrationTests(File f) {} + @Override public void libertyCreate() {} + @Override public void libertyDeploy() {} + @Override public void libertyInstallFeature() {} + @Override public boolean libertyGenerateFeatures(Collection c, boolean o) { return false; } + @Override public void redeployApp() {} + @Override public String getServerStartTimeoutExample() { return null; } + @Override public String getProjectName() { return null; } + @Override public boolean isLooseApplication() { return true; } + @Override public File getLooseApplicationFile() { return null; } + @Override public boolean compile(File d, ProjectModule p) { return false; } + @Override protected void updateLooseApp() {} + @Override protected void resourceDirectoryCreated() {} + @Override protected void resourceModifiedOrCreated(File f, File r, File o) {} + @Override protected void resourceDeleted(File f, File r, File o) {} + } + + @Test + public void testGenerateContainerName_noExistingContainers() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", null); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev-modulea", name); + } + + @Test + public void testGenerateContainerName_preferredNameFree() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "some-other-container liberty-dev-moduleb"); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev-modulea", name); + } + + @Test + public void testGenerateContainerName_preferredNameTaken_firstSuffix() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "liberty-dev-modulea"); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev-modulea-1", name); + } + + @Test + public void testGenerateContainerName_preferredNameAndFirstSuffixTaken() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "liberty-dev-modulea liberty-dev-modulea-1"); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev-modulea-2", name); + } + + @Test + public void testGenerateContainerName_differentModulesGetDifferentNames() throws Exception { + ContainerNameTestUtil utilA = new ContainerNameTestUtil("moduleA", null); + ContainerNameTestUtil utilB = new ContainerNameTestUtil("moduleB", null); + assertNotEquals(utilA.callGenerateNewContainerName(), utilB.callGenerateNewContainerName()); + } + + @Test + public void testGenerateContainerName_nullApplicationId() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil(null, null); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev", name); + } + + @Test + public void testGenerateContainerName_applicationIdWithSpecialChars() throws Exception { + ContainerNameTestUtil util = new ContainerNameTestUtil("com.example:my-App", null); + String name = util.callGenerateNewContainerName(); + assertEquals("liberty-dev-com.example-my-app", name); + } +} diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java new file mode 100644 index 00000000..348db624 --- /dev/null +++ b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java @@ -0,0 +1,177 @@ +/** + * (C) Copyright IBM Corporation 2026. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.openliberty.tools.common.plugins.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Method; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.ServerSocket; +import java.util.Collection; +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.ThreadPoolExecutor; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +public class DevUtilPortHoldingTest { + + @Rule + public TemporaryFolder temp = new TemporaryFolder(); + + private class PortTestUtil extends DevUtil { + + PortTestUtil() throws IOException { + super(temp.newFolder(), + null, null, null, null, null, null, + null, false, false, false, false, false, false, + "test-app", 30, 30, 5, 500, + true, false, false, false, + false, null, null, null, 0, false, null, false, + null, null, false, null, null, null, false, + null, null, null, Collections.emptyMap()); + } + + ServerSocket callBindPortSocket(int port) throws Exception { + Method m = DevUtil.class.getDeclaredMethod("bindPortSocket", int.class); + m.setAccessible(true); + return (ServerSocket) m.invoke(this, port); + } + + @Override protected String execContainerCmdWithPrefix(String cmd, int t, boolean e) { return null; } + @Override public void debug(String msg) {} + @Override public void debug(String msg, Throwable e) {} + @Override public void debug(Throwable e) {} + @Override public void warn(String msg) {} + @Override public void info(String msg) {} + @Override public void error(String msg) {} + @Override public void error(String msg, Throwable e) {} + @Override public boolean isDebugEnabled() { return false; } + @Override public void stopServer() {} + @Override public io.openliberty.tools.ant.ServerTask getServerTask() { return null; } + @Override public boolean recompileBuildFile(File f, Set c, Set t, boolean g, ThreadPoolExecutor e) { return false; } + @Override public boolean updateArtifactPaths(ProjectModule m, boolean r, boolean g, ThreadPoolExecutor e) { return false; } + @Override public boolean updateArtifactPaths(File f) { return false; } + @Override public int countApplicationUpdatedMessages() { return 0; } + @Override public void runTests(boolean w, int m, ThreadPoolExecutor e, boolean a, boolean b, boolean c, File f, String s) {} + @Override public void installFeatures(File f, File s, boolean g) {} + @Override public ServerFeatureUtil getServerFeatureUtilObj() { return null; } + @Override public Set getExistingFeatures() { return null; } + @Override public void updateExistingFeatures() {} + @Override public boolean compile(File d) { return false; } + @Override public void runUnitTests(File f) {} + @Override public void runIntegrationTests(File f) {} + @Override public void libertyCreate() {} + @Override public void libertyDeploy() {} + @Override public void libertyInstallFeature() {} + @Override public boolean libertyGenerateFeatures(Collection c, boolean o) { return false; } + @Override public void redeployApp() {} + @Override public String getServerStartTimeoutExample() { return null; } + @Override public String getProjectName() { return null; } + @Override public boolean isLooseApplication() { return true; } + @Override public File getLooseApplicationFile() { return null; } + @Override public boolean compile(File d, ProjectModule p) { return false; } + @Override protected void updateLooseApp() {} + @Override protected void resourceDirectoryCreated() {} + @Override protected void resourceModifiedOrCreated(File f, File r, File o) {} + @Override protected void resourceDeleted(File f, File r, File o) {} + } + + private static int findFreePort() throws IOException { + try (ServerSocket s = new ServerSocket(0)) { + s.setReuseAddress(true); + return s.getLocalPort(); + } + } + + @Test + public void testFindAvailablePort_returnsFreePort() throws Exception { + PortTestUtil util = new PortTestUtil(); + int freePort = findFreePort(); + int found = util.findAvailablePort(freePort, false); + assertTrue("findAvailablePort must return a port >= 1024", found >= 1024); + } + + @Test + public void testBindPortSocket_succeeds_onFreePort() throws Exception { + PortTestUtil util = new PortTestUtil(); + int port = findFreePort(); + ServerSocket sock = util.callBindPortSocket(port); + try { + assertNotNull("bindPortSocket should return a non-null socket for a free port", sock); + assertEquals("Bound port must match requested port", port, sock.getLocalPort()); + } finally { + if (sock != null) sock.close(); + } + } + + @Test + public void testBindPortSocket_returnsNull_whenPortAlreadyBound() throws Exception { + PortTestUtil util = new PortTestUtil(); + int port = findFreePort(); + try (ServerSocket occupier = new ServerSocket()) { + occupier.setReuseAddress(false); + occupier.bind(new InetSocketAddress(InetAddress.getByName(null), port), 1); + + ServerSocket result = util.callBindPortSocket(port); + assertNull("bindPortSocket must return null when the port is already in use", result); + } + } + + @Test + public void testPortHeldByBindSocket_notReturnedByFindAvailablePort() throws Exception { + PortTestUtil util = new PortTestUtil(); + int port = findFreePort(); + ServerSocket held = util.callBindPortSocket(port); + assertNotNull("Pre-condition: bindPortSocket should succeed on a free port", held); + try { + int next = util.findAvailablePort(port, false); + assertNotEquals( + "findAvailablePort must not return a port that is already held open", + port, next); + } finally { + held.close(); + } + } + + @Test + public void testTwoSequentialFindAvailablePort_returnDifferentPorts_whenFirstIsHeld() throws Exception { + PortTestUtil util = new PortTestUtil(); + int preferredPort = findFreePort(); + + int portA = util.findAvailablePort(preferredPort, false); + ServerSocket holdA = util.callBindPortSocket(portA); + assertNotNull("Module A must be able to hold its chosen port", holdA); + + try { + int portB = util.findAvailablePort(preferredPort, false); + assertNotEquals( + "Module B must receive a different port from module A's held port", + portA, portB); + } finally { + holdA.close(); + } + } +} From f222689d44ae56d61e17ca00c58f46212b33c1fc Mon Sep 17 00:00:00 2001 From: Arun Venmany Date: Thu, 3 Sep 2026 12:20:31 +0530 Subject: [PATCH 2/4] Refactor test utilities: remove duplicate DevUtil subclasses --- .../tools/common/plugins/util/DevUtil.java | 2 +- .../common/plugins/util/BaseDevUtilTest.java | 29 ++++- .../util/DevUtilContainerNameTest.java | 116 +++--------------- .../plugins/util/DevUtilPortHoldingTest.java | 81 +----------- 4 files changed, 48 insertions(+), 180 deletions(-) diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java index d61216e5..23229699 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java @@ -1853,7 +1853,7 @@ private void closeQuietly(ServerSocket s) { * SO_REUSEADDR is explicitly set to false so that the OS will not allow another * process to bind the same port while this socket is open. */ - private ServerSocket bindPortSocket(int port) { + ServerSocket bindPortSocket(int port) { try { if (OSUtil.isWindows()) { return new ServerSocket(port); diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/BaseDevUtilTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/BaseDevUtilTest.java index 50f7de39..e34a1677 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/BaseDevUtilTest.java +++ b/src/test/java/io/openliberty/tools/common/plugins/util/BaseDevUtilTest.java @@ -18,9 +18,10 @@ import java.io.File; import java.io.IOException; +import java.net.ServerSocket; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; -import java.nio.file.Path; import java.util.Collections; import java.util.List; import java.util.Set; @@ -38,6 +39,8 @@ public class BaseDevUtilTest { public class DevTestUtil extends DevUtil { + private String containerCmdOutput = null; + public DevTestUtil(File serverDirectory, File sourceDirectory, File testSourceDirectory, File configDirectory, List resourceDirs, List webResourceDirs, boolean hotTests, boolean skipTests) throws IOException { super(temp.newFolder(), serverDirectory, sourceDirectory, testSourceDirectory, configDirectory, null, null, @@ -57,12 +60,28 @@ public DevTestUtil(File serverDirectory, File buildDir, boolean container) { container, null, null, null, 0, false, null, false, null, null, false, null, null, null, false, null, null, null, Collections.emptyMap()); } + public DevTestUtil(File buildDir, String applicationId, String containerCmdOutput) throws IOException { + super(buildDir, null, null, null, null, null, null, + null, false, false, false, false, false, false, applicationId, 30, 30, 5, 500, true, false, false, false, + false, null, null, null, 0, false, null, false, null, null, false, null, null, null, false, null, null, null, Collections.emptyMap()); + this.containerCmdOutput = containerCmdOutput; + } + /** - * Returns null for all container CLI commands to avoid requiring a running container engine. + * Returns null for all container CLI commands to avoid requiring a running container engine, + * unless a fake output has been set via the applicationId constructor. */ @Override protected String execContainerCmdWithPrefix(String command, int timeout, boolean throwExceptionOnError) { - return null; + return containerCmdOutput; + } + + public ServerSocket callBindPortSocket(int port) { + return bindPortSocket(port); + } + + public String callGenerateNewContainerName() throws PluginExecutionException { + return generateNewContainerName(); } /** @@ -307,4 +326,8 @@ public DevUtil getNewDevUtil(File serverDirectory, File buildDir) { public DevTestUtil getNewContainerUtil() { return new DevTestUtil(null, null, true); } + + public DevTestUtil getNewDevUtil(File buildDir, String applicationId, String containerCmdOutput) throws IOException { + return new DevTestUtil(buildDir, applicationId, containerCmdOutput); + } } diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java index f2da622b..801f15c0 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java +++ b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java @@ -19,21 +19,9 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; -import java.io.File; -import java.io.IOException; -import java.util.Collection; -import java.util.Collections; -import java.util.Set; -import java.util.concurrent.ThreadPoolExecutor; - -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.TemporaryFolder; -public class DevUtilContainerNameTest { - - @Rule - public TemporaryFolder temp = new TemporaryFolder(); +public class DevUtilContainerNameTest extends BaseDevUtilTest { @Test public void testSanitize_null() { @@ -93,120 +81,46 @@ public void testSanitize_upperCaseLowered() { assertEquals("moduleabc", DevUtil.sanitizeContainerNameSegment("ModuleABC")); } - private class ContainerNameTestUtil extends DevUtil { - - private final String fakeContainerList; - - ContainerNameTestUtil(String applicationId, String fakeContainerList) throws IOException { - super(temp.newFolder(), - null, - null, null, null, - null, null, - null, - false, false, false, false, false, false, - applicationId, - 30, 30, 5, 500, - true, false, false, false, - false, - null, null, null, 0, false, null, false, - null, null, false, null, null, null, false, - null, null, null, Collections.emptyMap()); - this.fakeContainerList = fakeContainerList; - } - - public String callGenerateNewContainerName() throws PluginExecutionException { - return generateNewContainerName(); - } - - @Override - protected String execContainerCmdWithPrefix(String command, int timeout, boolean throwExceptionOnError) { - return fakeContainerList; - } - - @Override public void debug(String msg) {} - @Override public void debug(String msg, Throwable e) {} - @Override public void debug(Throwable e) {} - @Override public void warn(String msg) {} - @Override public void info(String msg) {} - @Override public void error(String msg) {} - @Override public void error(String msg, Throwable e) {} - @Override public boolean isDebugEnabled() { return false; } - @Override public void stopServer() {} - @Override public io.openliberty.tools.ant.ServerTask getServerTask() { return null; } - @Override public boolean recompileBuildFile(File f, Set c, Set t, boolean g, ThreadPoolExecutor e) { return false; } - @Override public boolean updateArtifactPaths(ProjectModule m, boolean r, boolean g, ThreadPoolExecutor e) { return false; } - @Override public boolean updateArtifactPaths(File f) { return false; } - @Override public int countApplicationUpdatedMessages() { return 0; } - @Override public void runTests(boolean w, int m, ThreadPoolExecutor e, boolean a, boolean b, boolean c, File f, String s) {} - @Override public void installFeatures(File f, File s, boolean g) {} - @Override public ServerFeatureUtil getServerFeatureUtilObj() { return null; } - @Override public Set getExistingFeatures() { return null; } - @Override public void updateExistingFeatures() {} - @Override public boolean compile(File d) { return false; } - @Override public void runUnitTests(File f) {} - @Override public void runIntegrationTests(File f) {} - @Override public void libertyCreate() {} - @Override public void libertyDeploy() {} - @Override public void libertyInstallFeature() {} - @Override public boolean libertyGenerateFeatures(Collection c, boolean o) { return false; } - @Override public void redeployApp() {} - @Override public String getServerStartTimeoutExample() { return null; } - @Override public String getProjectName() { return null; } - @Override public boolean isLooseApplication() { return true; } - @Override public File getLooseApplicationFile() { return null; } - @Override public boolean compile(File d, ProjectModule p) { return false; } - @Override protected void updateLooseApp() {} - @Override protected void resourceDirectoryCreated() {} - @Override protected void resourceModifiedOrCreated(File f, File r, File o) {} - @Override protected void resourceDeleted(File f, File r, File o) {} - } - @Test public void testGenerateContainerName_noExistingContainers() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", null); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev-modulea", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "moduleA", null); + assertEquals("liberty-dev-modulea", util.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_preferredNameFree() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "some-other-container liberty-dev-moduleb"); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev-modulea", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "moduleA", "some-other-container liberty-dev-moduleb"); + assertEquals("liberty-dev-modulea", util.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_preferredNameTaken_firstSuffix() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "liberty-dev-modulea"); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev-modulea-1", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "moduleA", "liberty-dev-modulea"); + assertEquals("liberty-dev-modulea-1", util.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_preferredNameAndFirstSuffixTaken() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil("moduleA", "liberty-dev-modulea liberty-dev-modulea-1"); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev-modulea-2", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "moduleA", "liberty-dev-modulea liberty-dev-modulea-1"); + assertEquals("liberty-dev-modulea-2", util.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_differentModulesGetDifferentNames() throws Exception { - ContainerNameTestUtil utilA = new ContainerNameTestUtil("moduleA", null); - ContainerNameTestUtil utilB = new ContainerNameTestUtil("moduleB", null); + DevTestUtil utilA = getNewDevUtil(temp.newFolder(), "moduleA", null); + DevTestUtil utilB = getNewDevUtil(temp.newFolder(), "moduleB", null); assertNotEquals(utilA.callGenerateNewContainerName(), utilB.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_nullApplicationId() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil(null, null); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), null, null); + assertEquals("liberty-dev", util.callGenerateNewContainerName()); } @Test public void testGenerateContainerName_applicationIdWithSpecialChars() throws Exception { - ContainerNameTestUtil util = new ContainerNameTestUtil("com.example:my-App", null); - String name = util.callGenerateNewContainerName(); - assertEquals("liberty-dev-com.example-my-app", name); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "com.example:my-App", null); + assertEquals("liberty-dev-com.example-my-app", util.callGenerateNewContainerName()); } } diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java index 348db624..be5734dc 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java +++ b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilPortHoldingTest.java @@ -21,83 +21,14 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import java.io.File; import java.io.IOException; -import java.lang.reflect.Method; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.ServerSocket; -import java.util.Collection; -import java.util.Collections; -import java.util.Set; -import java.util.concurrent.ThreadPoolExecutor; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.TemporaryFolder; -public class DevUtilPortHoldingTest { - - @Rule - public TemporaryFolder temp = new TemporaryFolder(); - - private class PortTestUtil extends DevUtil { - - PortTestUtil() throws IOException { - super(temp.newFolder(), - null, null, null, null, null, null, - null, false, false, false, false, false, false, - "test-app", 30, 30, 5, 500, - true, false, false, false, - false, null, null, null, 0, false, null, false, - null, null, false, null, null, null, false, - null, null, null, Collections.emptyMap()); - } - - ServerSocket callBindPortSocket(int port) throws Exception { - Method m = DevUtil.class.getDeclaredMethod("bindPortSocket", int.class); - m.setAccessible(true); - return (ServerSocket) m.invoke(this, port); - } - - @Override protected String execContainerCmdWithPrefix(String cmd, int t, boolean e) { return null; } - @Override public void debug(String msg) {} - @Override public void debug(String msg, Throwable e) {} - @Override public void debug(Throwable e) {} - @Override public void warn(String msg) {} - @Override public void info(String msg) {} - @Override public void error(String msg) {} - @Override public void error(String msg, Throwable e) {} - @Override public boolean isDebugEnabled() { return false; } - @Override public void stopServer() {} - @Override public io.openliberty.tools.ant.ServerTask getServerTask() { return null; } - @Override public boolean recompileBuildFile(File f, Set c, Set t, boolean g, ThreadPoolExecutor e) { return false; } - @Override public boolean updateArtifactPaths(ProjectModule m, boolean r, boolean g, ThreadPoolExecutor e) { return false; } - @Override public boolean updateArtifactPaths(File f) { return false; } - @Override public int countApplicationUpdatedMessages() { return 0; } - @Override public void runTests(boolean w, int m, ThreadPoolExecutor e, boolean a, boolean b, boolean c, File f, String s) {} - @Override public void installFeatures(File f, File s, boolean g) {} - @Override public ServerFeatureUtil getServerFeatureUtilObj() { return null; } - @Override public Set getExistingFeatures() { return null; } - @Override public void updateExistingFeatures() {} - @Override public boolean compile(File d) { return false; } - @Override public void runUnitTests(File f) {} - @Override public void runIntegrationTests(File f) {} - @Override public void libertyCreate() {} - @Override public void libertyDeploy() {} - @Override public void libertyInstallFeature() {} - @Override public boolean libertyGenerateFeatures(Collection c, boolean o) { return false; } - @Override public void redeployApp() {} - @Override public String getServerStartTimeoutExample() { return null; } - @Override public String getProjectName() { return null; } - @Override public boolean isLooseApplication() { return true; } - @Override public File getLooseApplicationFile() { return null; } - @Override public boolean compile(File d, ProjectModule p) { return false; } - @Override protected void updateLooseApp() {} - @Override protected void resourceDirectoryCreated() {} - @Override protected void resourceModifiedOrCreated(File f, File r, File o) {} - @Override protected void resourceDeleted(File f, File r, File o) {} - } +public class DevUtilPortHoldingTest extends BaseDevUtilTest { private static int findFreePort() throws IOException { try (ServerSocket s = new ServerSocket(0)) { @@ -108,7 +39,7 @@ private static int findFreePort() throws IOException { @Test public void testFindAvailablePort_returnsFreePort() throws Exception { - PortTestUtil util = new PortTestUtil(); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "test-app", null); int freePort = findFreePort(); int found = util.findAvailablePort(freePort, false); assertTrue("findAvailablePort must return a port >= 1024", found >= 1024); @@ -116,7 +47,7 @@ public void testFindAvailablePort_returnsFreePort() throws Exception { @Test public void testBindPortSocket_succeeds_onFreePort() throws Exception { - PortTestUtil util = new PortTestUtil(); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "test-app", null); int port = findFreePort(); ServerSocket sock = util.callBindPortSocket(port); try { @@ -129,7 +60,7 @@ public void testBindPortSocket_succeeds_onFreePort() throws Exception { @Test public void testBindPortSocket_returnsNull_whenPortAlreadyBound() throws Exception { - PortTestUtil util = new PortTestUtil(); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "test-app", null); int port = findFreePort(); try (ServerSocket occupier = new ServerSocket()) { occupier.setReuseAddress(false); @@ -142,7 +73,7 @@ public void testBindPortSocket_returnsNull_whenPortAlreadyBound() throws Excepti @Test public void testPortHeldByBindSocket_notReturnedByFindAvailablePort() throws Exception { - PortTestUtil util = new PortTestUtil(); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "test-app", null); int port = findFreePort(); ServerSocket held = util.callBindPortSocket(port); assertNotNull("Pre-condition: bindPortSocket should succeed on a free port", held); @@ -158,7 +89,7 @@ public void testPortHeldByBindSocket_notReturnedByFindAvailablePort() throws Exc @Test public void testTwoSequentialFindAvailablePort_returnDifferentPorts_whenFirstIsHeld() throws Exception { - PortTestUtil util = new PortTestUtil(); + DevTestUtil util = getNewDevUtil(temp.newFolder(), "test-app", null); int preferredPort = findFreePort(); int portA = util.findAvailablePort(preferredPort, false); From 59223a85cbcd87cf002c4ea0cdfe40daebb64752 Mon Sep 17 00:00:00 2001 From: Arun Venmany Date: Thu, 3 Sep 2026 18:51:01 +0530 Subject: [PATCH 3/4] changes based on review comments --- .../tools/common/plugins/util/DevUtil.java | 58 ++++++++++++++----- .../util/DevUtilContainerNameTest.java | 4 +- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java index 23229699..5a15bdab 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java @@ -1795,10 +1795,15 @@ private String[] getContainerCommand() throws IOException, PluginExecutionExcept info("Container command: "+String.join(" ", newCommand)); // Release all held port sockets as late as possible so the container engine // can bind those exact ports immediately after this command is issued. - for (ServerSocket s : heldSockets) { - closeQuietly(s); + // The finally block ensures sockets are released even if an exception is thrown + // above (e.g. from addCopiedFiles, addUserId, or addContainerRunOpts). + try { + return newCommand; + } finally { + for (ServerSocket s : heldSockets) { + closeQuietly(s); + } } - return newCommand; } /** @@ -1891,9 +1896,27 @@ private String getContainerOption(String optionName) { return null; } + /** + * Generates a unique container name for this dev mode instance. + * + *

The preferred name is {@code "liberty-dev-"} (or just + * {@code "liberty-dev"} when {@code applicationId} is null or blank). Each module in a + * multi-module project therefore gets a stable, distinct default name that does not + * depend on the order in which modules start, eliminating the race condition that + * occurred when multiple modules computed the same numeric suffix from a shared + * container list. + * + *

If the preferred name is already in use by an existing container (e.g. a second + * instance of the same module, or two different apps that happen to share the same + * application ID), an incrementing numeric suffix is appended until an unused name is + * found (e.g. {@code "liberty-dev-modulea-1"}). + * + * @return a unique container name that is not currently used by any running or stopped container + * @throws PluginExecutionException if the container command cannot be executed + */ String generateNewContainerName() throws PluginExecutionException { // Build a sanitized name segment from applicationId so each module gets a stable, - // unique default name (e.g. "liberty-dev-moduleA") that does not depend on the + // unique default name (e.g. "liberty-dev-modulea") that does not depend on the // order in which modules start. This eliminates the race condition that occurred // when multiple modules computed the same numeric suffix from a shared container list. String appSegment = sanitizeContainerNameSegment(applicationId); @@ -1927,11 +1950,12 @@ String generateNewContainerName() throws PluginExecutionException { /** * Sanitizes a string so it can be used as part of a container name. - * Container names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*. - * Characters outside that set are replaced with hyphens, leading/trailing - * hyphens are removed, and the result is truncated to 63 characters so - * that the full "liberty-dev-<segment>" name stays within the 128-char limit - * that most container engines impose on container names. + * Container names must match {@code [a-zA-Z0-9][a-zA-Z0-9_.-]*}. + * The input is lower-cased, characters outside the allowed set are replaced + * with hyphens, consecutive separator runs are collapsed to a single hyphen, + * leading/trailing separators are removed, and the result is truncated to + * 110 characters so that the full {@code "liberty-dev-"} name stays + * within the 128-char limit that most container engines impose on container names. * * @param input the raw string to sanitize (e.g. applicationId) * @return a sanitized, lower-case string suitable for use in a container name, @@ -1941,15 +1965,17 @@ static String sanitizeContainerNameSegment(String input) { if (input == null || input.trim().isEmpty()) { return ""; } - // Replace any character that is not alphanumeric, hyphen, underscore, or dot with a hyphen. - String sanitized = input.trim().toLowerCase().replaceAll("[^a-z0-9_.]", "-"); - // Collapse consecutive hyphens/dots/underscores to a single hyphen. + // Lower-case the input and replace any character that is not alphanumeric, + // hyphen, underscore, or dot with a hyphen. + String sanitized = input.trim().toLowerCase().replaceAll("[^a-z0-9_.\\-]", "-"); + // Collapse consecutive separators (hyphens, dots, underscores) to a single hyphen. sanitized = sanitized.replaceAll("[-_.]{2,}", "-"); - // Remove leading and trailing hyphens. + // Remove leading and trailing separators (hyphens, dots, underscores). sanitized = sanitized.replaceAll("^[-_.]+|[-_.]+$", ""); - // Truncate to 63 characters. - if (sanitized.length() > 63) { - sanitized = sanitized.substring(0, 63); + // Truncate to 110 characters so the full "liberty-dev-" name stays + // within the 128-char limit that most container engines impose on container names. + if (sanitized.length() > 110) { + sanitized = sanitized.substring(0, 110); sanitized = sanitized.replaceAll("[-_.]+$", ""); } return sanitized; diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java index 801f15c0..73f7c04d 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java +++ b/src/test/java/io/openliberty/tools/common/plugins/util/DevUtilContainerNameTest.java @@ -71,9 +71,9 @@ public void testSanitize_leadingAndTrailingSeparatorsRemoved() { @Test public void testSanitize_truncatesLongInput() { StringBuilder sb = new StringBuilder(); - for (int i = 0; i < 70; i++) sb.append('a'); + for (int i = 0; i < 120; i++) sb.append('a'); String result = DevUtil.sanitizeContainerNameSegment(sb.toString()); - assertTrue("Sanitized segment must be <= 63 chars", result.length() <= 63); + assertTrue("Sanitized segment must be <= 110 chars", result.length() <= 110); } @Test From 4734cf144c18fc9078e780059f1feb782aa68660 Mon Sep 17 00:00:00 2001 From: Arun Venmany Date: Thu, 3 Sep 2026 19:25:43 +0530 Subject: [PATCH 4/4] Wrap try/finally around findAndHoldPort calls so held sockets are released close to their acquisition point, not at the end of the method --- .../tools/common/plugins/util/DevUtil.java | 203 +++++++++--------- 1 file changed, 101 insertions(+), 102 deletions(-) diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java index 5a15bdab..f104995b 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/DevUtil.java @@ -1689,115 +1689,114 @@ private String[] getContainerCommand() throws IOException, PluginExecutionExcept // Holding all three sockets simultaneously means a concurrent module's // findAvailablePort() will see each chosen port as already bound and advance to // the next one, eliminating the TOCTOU race without any shared state. + // The try/finally below ensures all sockets are released even if an exception + // is thrown while building the rest of the command. List heldSockets = new ArrayList(); + try { + if (!skipDefaultPorts) { + int httpPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTP_PORT, false, heldSockets); + int httpsPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTPS_PORT, false, heldSockets); + commandElements.add("-p"); + commandElements.add(httpPortToUse+":"+LIBERTY_DEFAULT_HTTP_PORT); - if (!skipDefaultPorts) { - int httpPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTP_PORT, false, heldSockets); - int httpsPortToUse = findAndHoldPort(LIBERTY_DEFAULT_HTTPS_PORT, false, heldSockets); - commandElements.add("-p"); - commandElements.add(httpPortToUse+":"+LIBERTY_DEFAULT_HTTP_PORT); - - commandElements.add("-p"); - commandElements.add(httpsPortToUse+":"+LIBERTY_DEFAULT_HTTPS_PORT); - } + commandElements.add("-p"); + commandElements.add(httpsPortToUse+":"+LIBERTY_DEFAULT_HTTPS_PORT); + } - if (libertyDebug) { - // map debug port - int containerDebugPort, hostDebugPort; - if (alternativeDebugPort == -1) { - // it is possible another JVM has grabbed our port since dev mode last checked - hostDebugPort = findAndHoldPort(libertyDebugPort, true, heldSockets); - containerDebugPort = libertyDebugPort; + if (libertyDebug) { + // map debug port + int containerDebugPort, hostDebugPort; + if (alternativeDebugPort == -1) { + // it is possible another JVM has grabbed our port since dev mode last checked + hostDebugPort = findAndHoldPort(libertyDebugPort, true, heldSockets); + containerDebugPort = libertyDebugPort; + } else { + // dev mode has already selected an ephemeral port + containerDebugPort = hostDebugPort = alternativeDebugPort; + } + commandElements.add("-p"); + commandElements.add(hostDebugPort+":"+containerDebugPort); + // set environment variables in the container to ensure debug mode does not suspend the server, and to enable a custom debug port to be used + // and to allow remote debugging into the container + commandElements.add("-e"); + commandElements.add("WLP_DEBUG_SUSPEND=n"); + commandElements.add("-e"); + commandElements.add("WLP_DEBUG_ADDRESS=" + containerDebugPort); + commandElements.add("-e"); + commandElements.add("WLP_DEBUG_REMOTE=y"); + } + + // mount potential directories containing .war.xml from devc specific folder - override /config/apps and /config/dropins + File tempApps = new File(buildDirectory, DEVC_HIDDEN_FOLDER + "/apps"); + File tempDropins = new File(buildDirectory, DEVC_HIDDEN_FOLDER + "/dropins"); + commandElements.add("-v"); + commandElements.add(tempApps + ":/config/apps"); + + commandElements.add("-v"); + commandElements.add(tempDropins + ":/config/dropins"); + + // mount the loose application resources in the container using the appropriate project root + File looseApplicationProjectRoot = getLooseAppProjectRoot(projectDirectory, multiModuleProjectDirectory); + commandElements.add("-v"); + commandElements.add(looseApplicationProjectRoot.getAbsolutePath() + ":" + DEVMODE_DIR_NAME); + + // mount the server logs directory over the /logs used by the open liberty container as defined by the LOG_DIR env. var. + File logsDir = new File(serverDirectory.getAbsolutePath(), "logs"); + commandElements.add("-v"); + commandElements.add(logsDir + ":/logs"); + + // mount the Maven .m2 cache directory for featureUtility to use. For now, featureUtility does not support Gradle cache. + commandElements.add("-v"); + commandElements.add(mavenCacheLocation + ":/devmode-maven-cache"); + + // mount all files from COPY commands in the Containerfile to allow for hot deployment + addCopiedFiles(commandElements); + + // Add a --user option when running Linux + addUserId(commandElements); + + // Do not generate a name if the user has specified a name + String name = getContainerOption("--name"); + if (name == null || name.isEmpty()) { + if (name != null && name.isEmpty()) { + error("The container option --name is specified with an unsupported value: empty string."); + // now generate a name so that the container errors make some sense to the user. + } + containerName = generateNewContainerName(); + commandElements.add("--name"); + commandElements.add(containerName); } else { - // dev mode has already selected an ephemeral port - containerDebugPort = hostDebugPort = alternativeDebugPort; - } - commandElements.add("-p"); - commandElements.add(hostDebugPort+":"+containerDebugPort); - // set environment variables in the container to ensure debug mode does not suspend the server, and to enable a custom debug port to be used - // and to allow remote debugging into the container - commandElements.add("-e"); - commandElements.add("WLP_DEBUG_SUSPEND=n"); - commandElements.add("-e"); - commandElements.add("WLP_DEBUG_ADDRESS=" + containerDebugPort); - commandElements.add("-e"); - commandElements.add("WLP_DEBUG_REMOTE=y"); - } - - // mount potential directories containing .war.xml from devc specific folder - override /config/apps and /config/dropins - File tempApps = new File(buildDirectory, DEVC_HIDDEN_FOLDER + "/apps"); - File tempDropins = new File(buildDirectory, DEVC_HIDDEN_FOLDER + "/dropins"); - commandElements.add("-v"); - commandElements.add(tempApps + ":/config/apps"); - - commandElements.add("-v"); - commandElements.add(tempDropins + ":/config/dropins"); - - // mount the loose application resources in the container using the appropriate project root - File looseApplicationProjectRoot = getLooseAppProjectRoot(projectDirectory, multiModuleProjectDirectory); - commandElements.add("-v"); - commandElements.add(looseApplicationProjectRoot.getAbsolutePath() + ":" + DEVMODE_DIR_NAME); - - // mount the server logs directory over the /logs used by the open liberty container as defined by the LOG_DIR env. var. - File logsDir = new File(serverDirectory.getAbsolutePath(), "logs"); - commandElements.add("-v"); - commandElements.add(logsDir + ":/logs"); - - // mount the Maven .m2 cache directory for featureUtility to use. For now, featureUtility does not support Gradle cache. - commandElements.add("-v"); - commandElements.add(mavenCacheLocation + ":/devmode-maven-cache"); - - // mount all files from COPY commands in the Containerfile to allow for hot deployment - addCopiedFiles(commandElements); - - // Add a --user option when running Linux - addUserId(commandElements); - - // Do not generate a name if the user has specified a name - String name = getContainerOption("--name"); - if (name == null || name.isEmpty()) { - if (name != null && name.isEmpty()) { - error("The container option --name is specified with an unsupported value: empty string."); - // now generate a name so that the container errors make some sense to the user. - } - containerName = generateNewContainerName(); - commandElements.add("--name"); - commandElements.add(containerName); - } else { - containerName = name; - } - debug("containerName: " + containerName + "."); + containerName = name; + } + debug("containerName: " + containerName + "."); - // Allow the user to add their own options to this command via a system property. - if (containerRunOpts != null) { - addContainerRunOpts(containerRunOpts, commandElements); - } + // Allow the user to add their own options to this command via a system property. + if (containerRunOpts != null) { + addContainerRunOpts(containerRunOpts, commandElements); + } - // Options must precede this in any order. Image name and command code follows. - commandElements.add(imageName); + // Options must precede this in any order. Image name and command code follows. + commandElements.add(imageName); - // Command to start the server - commandElements.add("server"); - if (libertyDebug) { - commandElements.add("debug"); - } else { - commandElements.add("run"); - } - commandElements.add("defaultServer"); - - // All the Liberty variable definitions must appear after the -- option. - // Important: other Liberty options must appear before -- - commandElements.add("--"); - commandElements.add("--"+DEVMODE_PROJECT_ROOT+"="+DEVMODE_DIR_NAME); - - //return command.toString(); - String[] newCommand = commandElements.toArray(new String[commandElements.size()]); - info("Container command: "+String.join(" ", newCommand)); - // Release all held port sockets as late as possible so the container engine - // can bind those exact ports immediately after this command is issued. - // The finally block ensures sockets are released even if an exception is thrown - // above (e.g. from addCopiedFiles, addUserId, or addContainerRunOpts). - try { + // Command to start the server + commandElements.add("server"); + if (libertyDebug) { + commandElements.add("debug"); + } else { + commandElements.add("run"); + } + commandElements.add("defaultServer"); + + // All the Liberty variable definitions must appear after the -- option. + // Important: other Liberty options must appear before -- + commandElements.add("--"); + commandElements.add("--"+DEVMODE_PROJECT_ROOT+"="+DEVMODE_DIR_NAME); + + //return command.toString(); + String[] newCommand = commandElements.toArray(new String[commandElements.size()]); + info("Container command: "+String.join(" ", newCommand)); + // Release the held port sockets as late as possible so the container engine + // can bind those exact ports immediately after this command is issued. return newCommand; } finally { for (ServerSocket s : heldSockets) {