[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #3555: Improve available port discovery in tests
ppalaga commented on a change in pull request #3555:
URL: https://github.com/apache/camel-quarkus/pull/3555#discussion_r807992801
##
File path:
integration-tests-support/test-support/src/main/java/org/apache/camel/quarkus/test/AvailablePortFinder.java
##
@@ -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()))
Review comment:
Shouldn't we make AvailablePortFinder class final, to better match the
assumption made on the above line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #3555: Improve available port discovery in tests
ppalaga commented on a change in pull request #3555:
URL: https://github.com/apache/camel-quarkus/pull/3555#discussion_r808019076
##
File path:
integration-tests-support/test-support/src/main/java/org/apache/camel/quarkus/test/AvailablePortFinder.java
##
@@ -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);
Review comment:
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 may exist in several
concurrent JVMs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #3555: Improve available port discovery in tests
ppalaga commented on a change in pull request #3555:
URL: https://github.com/apache/camel-quarkus/pull/3555#discussion_r808021037
##
File path:
integration-tests-support/test-support/src/main/java/org/apache/camel/quarkus/test/AvailablePortFinder.java
##
@@ -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()))
Review comment:
> The class declaration is:
>
> ```java
> public final class AvailablePortFinder {
> }
> ```
I am blind. Sorry for the noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #3555: Improve available port discovery in tests
ppalaga commented on a change in pull request #3555:
URL: https://github.com/apache/camel-quarkus/pull/3555#discussion_r808019076
##
File path:
integration-tests-support/test-support/src/main/java/org/apache/camel/quarkus/test/AvailablePortFinder.java
##
@@ -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);
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #3555: Improve available port discovery in tests
ppalaga commented on a change in pull request #3555:
URL: https://github.com/apache/camel-quarkus/pull/3555#discussion_r808289948
##
File path:
integration-tests-support/test-support/src/main/java/org/apache/camel/quarkus/test/AvailablePortFinder.java
##
@@ -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);
Review comment:
Thanks for explaining. If it is just for logging, indeed, multiple
RESERVED_PORTS instances in concurrent per-module JVMs do not matter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
