-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve available port discovery in tests #3555
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.function.Function; | ||
|
||
import org.eclipse.microprofile.config.Config; | ||
|
@@ -35,9 +36,11 @@ | |
*/ | ||
public final class AvailablePortFinder { | ||
private static final Logger LOGGER = LoggerFactory.getLogger(AvailablePortFinder.class); | ||
private static final Map<Integer, String> RESERVED_PORTS = new ConcurrentHashMap<>(); | ||
private static final String[] QUARKUS_PORT_PROPERTIES = new String[] { | ||
"quarkus.http.test-port", | ||
"quarkus.http.test-ssl-port", | ||
"quarkus.https.test-port", | ||
}; | ||
|
||
/** | ||
|
@@ -54,15 +57,23 @@ private AvailablePortFinder() { | |
* @return the available port | ||
*/ | ||
public static int getNextAvailable() { | ||
// Using AvailablePortFinder in native applications can be problematic | ||
// E.g The reserved port may be allocated at build time and preserved indefinitely at runtime. I.e it never changes on each execution of the native application | ||
logWarningIfNativeApplication(); | ||
|
||
while (true) { | ||
try (ServerSocket ss = new ServerSocket()) { | ||
ss.setReuseAddress(true); | ||
ss.bind(new InetSocketAddress((InetAddress) null, 0), 1); | ||
|
||
int port = ss.getLocalPort(); | ||
if (!isQuarkusReservedPort(port)) { | ||
LOGGER.info("getNextAvailable() -> {}", port); | ||
return port; | ||
String callerClassName = getCallerClassName(); | ||
String value = RESERVED_PORTS.putIfAbsent(port, callerClassName); | ||
if (value == null) { | ||
LOGGER.info("{} reserved port {}", callerClassName, port); | ||
return port; | ||
} | ||
} | ||
} catch (IOException e) { | ||
throw new IllegalStateException("Cannot find free port", e); | ||
|
@@ -90,6 +101,16 @@ public static <T> Map<String, T> reserveNetworkPorts(Function<Integer, T> conver | |
return reservedPorts; | ||
} | ||
|
||
public static void releaseReservedPorts() { | ||
String callerClassName = getCallerClassName(); | ||
RESERVED_PORTS.entrySet() | ||
.stream() | ||
.filter(entry -> entry.getValue().equals(callerClassName)) | ||
.peek(entry -> LOGGER.info("Releasing port {} reserved by {}", entry.getKey(), entry.getValue())) | ||
.map(Map.Entry::getKey) | ||
.forEach(RESERVED_PORTS::remove); | ||
} | ||
|
||
private static boolean isQuarkusReservedPort(int port) { | ||
Config config = ConfigProvider.getConfig(); | ||
for (String property : QUARKUS_PORT_PROPERTIES) { | ||
|
@@ -103,4 +124,19 @@ private static boolean isQuarkusReservedPort(int port) { | |
} | ||
return false; | ||
} | ||
|
||
private static String getCallerClassName() { | ||
return StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) | ||
.walk(s -> s.map(StackWalker.StackFrame::getClassName) | ||
.filter(className -> !className.equals(AvailablePortFinder.class.getName())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we make AvailablePortFinder class final, to better match the assumption made on the above line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The class declaration is: public final class AvailablePortFinder {
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filtering on className should be enough then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am blind. Sorry for the noise. |
||
.findFirst() | ||
aldettinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.orElseThrow(IllegalStateException::new)); | ||
} | ||
|
||
private static void logWarningIfNativeApplication() { | ||
if (System.getProperty("org.graalvm.nativeimage.kind") != null) { | ||
LOGGER.warn("Usage of AvailablePortFinder in the native application is discouraged. " | ||
+ "Pass the reserved port to the native application under test with QuarkusTestResource or via an HTTP request"); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,5 +76,6 @@ public void stop() { | |
} catch (Exception e) { | ||
// ignored | ||
} | ||
AvailablePortFinder.releaseReservedPorts(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You 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 org.apache.camel.quarkus.component.mllp.it; | ||
|
||
import java.util.Collections; | ||
import java.util.Map; | ||
|
||
import io.quarkus.test.common.QuarkusTestResourceLifecycleManager; | ||
import org.apache.camel.quarkus.test.AvailablePortFinder; | ||
|
||
public class MllpTestResource implements QuarkusTestResourceLifecycleManager { | ||
|
||
@Override | ||
public Map<String, String> start() { | ||
return Collections.singletonMap("mllp.test.port", Integer.toString(AvailablePortFinder.getNextAvailable())); | ||
} | ||
|
||
@Override | ||
public void stop() { | ||
AvailablePortFinder.releaseReservedPorts(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this making some assumptions about whether this class can concurrently exist in multiple JVMs or multiple class loaders within a single maven invocation? IIRC surefire is forking a new JVM for each Maven module. So if building with mvnd, the AvailablePortFinder class (and thus also separate RESERVED_PORTS instances) may exist in several concurrent JVMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a bit more? It's not entirely clear to me why it matters in this context.
All it's doing is trying to track which class reserved a specific port. Arguably we don't need to do that. It just provides some nice logging output and a way to clear out stale entries from the
RESERVED_PORTS
map.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - after re-reading. It's about separate
RESERVED_PORTS
instances.In which case, yes, I suppose the map contents would be different between concurrent JVMs or class loaders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. If it is just for logging, indeed, multiple RESERVED_PORTS instances in concurrent per-module JVMs do not matter.