gemmellr commented on code in PR #6021:
URL: https://github.com/apache/activemq-artemis/pull/6021#discussion_r2495208757
##########
artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/artemis-utility.profile:
##########
@@ -24,7 +24,7 @@ if [ -z "$LOGGING_ARGS" ]; then
fi
if [ -z "$JAVA_ARGS" ]; then
- JAVA_ARGS="-Dlog4j2.disableJmx=true --add-opens
java.base/jdk.internal.misc=ALL-UNNAMED ${java-utility-opts}"
+ JAVA_ARGS="-Dlog4j2.disableJmx=true --add-opens
java.base/jdk.internal.misc=ALL-UNNAMED --enable-native-access=ALL-UNNAMED
${java-utility-opts}"
Review Comment:
At least its the case that --enable-native-access was already in JDK17 (as
the initial FFM in 17 was an incubator addition there) so hopefully that means
it works on all versions of JDK17+ as opposed to just post-GA backport
releases, save us doing any checking.
##########
artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/bin/artemis:
##########
@@ -119,6 +119,9 @@ if [ -f "$ARTEMIS_OOME_DUMP" ] ; then
mv $ARTEMIS_OOME_DUMP $ARTEMIS_OOME_DUMP.bkp
fi
+# Netty needs access to unsafe, but this is turned off in Java 25 by default
Review Comment:
This comment could be confusing/misleading later, since Unsafe itself is
_not_ turned off in Java 25 by default. It warns once on first use on 24+ but
whats left of Unsafe (some has been removed already) is still usable. Setting
--sun-misc-unsafe-memory-access=allow facilitates stopping printing the warning
on Java 24+. On future JDKs, e.g perhaps 26+, that will not be permitted and
the Unsafe methods will be disabled by default and the allowed fallback will
become --sun-misc-unsafe-memory-access=warn to again permit them but always
with the warning. On yet further future JDKs the Unsafe memory methods will be
removed and --sun-misc-unsafe-memory-access will be ignored. At a later point
--sun-misc-unsafe-memory-access will be removed (which may mean its presence
prevents startup)
_Netty 4.2 doesnt use Unsafe by default_ in Java 24+ so as to avoid the
warning (whereas 4.1 still does use Unsafe by default [excepting
4.1.120-4.1.121], hence the warning mentioned on ARTEMIS 5711 when it uses
Unsafe currently). Instead it uses the 'fallback MemorySegment' based cleaner
on 25+ (its broken on 24, so that seems to default to heap-buffers rather than
direct buffers) rather than use Unsafe. Its possible to have it use a more
performant alternative on 24+ by explicitly enabling native access (which is
actually still enabled by default on 24+, for now, but again produces a
warning, so netty 4.2 doesnt do this by default to avoid those warnings,
requiring the --enable-native-access flag). Yet alternatively again, you can
make it just use Unsafe on Java 24+ [until they prevent 'allow', perhaps
JDK26+] by explicitly specifying it can via
--sun-misc-unsafe-memory-access=allow, or using the netty-specific
io.netty.noUnsafe prop.
https://github.com/netty/netty/blob/netty-4.2.7.Final/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L547-L588
plus the monster starting at
https://github.com/netty/netty/blob/4.2/common/src/main/java/io/netty/util/internal/PlatformDependent0.java#L85
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/SpawnedVMSupport.java:
##########
@@ -204,6 +204,12 @@ public static Process spawnVM(String classPath,
commandList.add(jacocoAgent);
}
+ String javaVersion = System.getProperty("java.version");
+ if (javaVersion.startsWith("24") || javaVersion.startsWith("25")) {
+ commandList.add("--enable-native-access=ALL-UNNAMED");
Review Comment:
Perhaps `Runtime.version().feature();` would work here.
Restricting to just 24 and 25 for --enable-native-access probably doesnt
make sense, its going to be useful or needed for all 26+
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/ActiveMQClientLogger.java:
##########
@@ -349,4 +349,10 @@ public interface ActiveMQClientLogger {
@LogMessage(id = 214036, value = "Connection closure to {} has been
detected: {} [code={}]", level = LogMessage.Level.INFO)
void connectionClosureDetected(String remoteAddress, String message,
ActiveMQExceptionType type);
+
+ @LogMessage(id = 214037, value = "Unable to check IoUring availability ",
level = LogMessage.Level.WARN)
+ void unableToCheckIoUringAvailability(Throwable e);
+
+ @LogMessage(id = 214038, value = "IoUring is not available, please add to
the classpath or configure useIoUring=false to remove this warning", level =
LogMessage.Level.WARN)
+ void unableToCheckIoUringAvailabilitynoClass();
Review Comment:
Could these go in commons? Its annoying when methods like these log and
suggest they are client issues, but are in fact being triggered by the broker
while its acting as a server to clients. E.g the method right above these ones
is especially confusing when logged by the broker.
##########
artemis-cli/pom.xml:
##########
@@ -239,6 +239,13 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <argLine>${activemq-surefire-argline}</argLine>
+ </configuration>
+ </plugin>
Review Comment:
Is this not already the case from the parent?
##########
artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/bin/artemis:
##########
@@ -119,6 +119,9 @@ if [ -f "$ARTEMIS_OOME_DUMP" ] ; then
mv $ARTEMIS_OOME_DUMP $ARTEMIS_OOME_DUMP.bkp
fi
+# Netty needs access to unsafe, but this is turned off in Java 25 by default
+$JAVACMD --sun-misc-unsafe-memory-access=allow --version > /dev/null 2>&1 &&
ALLOW_UNSAFE="--sun-misc-unsafe-memory-access=allow"
Review Comment:
If netty is the only thing we need/decide to enable Unsafe for (dont know)
on 25+, then I wonder if going with io.netty.noUnsafe sys prop might be
preferable, as it at least currently seems like it wont require a second JVM
startup and there would be no harm always specifying it on either older JDKs or
future JDKs after its irrelevant.
Though, that would leave the warning on 25..
I still wonder about just leaving it not using Unsafe on 25. Seems like that
should be possible, and that is the future beyond 25. If thats still not
viable, perhaps we stick with 4.1 using Unsafe and its warnings for a bit
longer until it is, and just try to ensure compatibility if an end user wants
to use 4.2 (the milestone/RC builds of spring using Artemis with 4.2 suggest it
mostly already is...thats the only 4.2 usage I'm personally aware of). Anyone
not satisifed with no-Unsafe can always add the config themselves.
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/ByteUtilTest.java:
##########
@@ -223,6 +223,7 @@ public void shouldZeroesDirectByteBuffer() {
@Test
public void shouldZeroesLimitedDirectByteBuffer() {
+ assumeTrue(PlatformDependent.hasUnsafe());
Review Comment:
I think this is possibly covering up a bug in either the test or more likely
in the ByteUtil that was exposed by Unsafe not being available, and so this
doesnt seem like the solution.
I dont think this test should need Unsafe, the behaviour just seems
questionable. The very next test calls the same utility method, without Unsafe.
It only behaves differently as the buffer passed isnt limited. Real code could
call the ByeUtil method later without Unsafe and get unexpected behaviour, it
wont have the benefit of covering itself with an assume.
EDIT: after typing that, I reminded myself of the time that this essentially
already happened and I fixed it in the calling code:
https://issues.apache.org/jira/browse/ARTEMIS-4340
https://lists.apache.org/thread/7scdf4859scywos4knw44dbdw39wphcb
If Franz doesnt know why he did it, and it doesnt work without Unsafe
anyway, I think it might be time we stopped it doing what its doing, or figure
out why it did and how to adapt it to not having Unsafe..
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/CheckDependencies.java:
##########
@@ -51,4 +52,17 @@ public static final boolean isKQueueAvailable() {
return false;
}
}
+
+ public static final boolean isIoUringAvailable() {
+ try {
+ return Env.isLinuxOs() && IoUring.isAvailable();
+ } catch (NoClassDefFoundError noClassDefFoundError) {
+ ActiveMQClientLogger.LOGGER.unableToCheckIoUringAvailabilitynoClass();
+ return false;
+ } catch (Throwable e) {
+ ActiveMQClientLogger.LOGGER.unableToCheckIoUringAvailability(e);
+ return false;
+ }
+ }
Review Comment:
This also feels like it would make more sense in commons (I know, the
previous checks are in here; same comment to them)
##########
artemis-core-client/pom.xml:
##########
@@ -89,6 +89,15 @@
<groupId>io.netty</groupId>
<artifactId>netty-transport-classes-kqueue</artifactId>
</dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-native-io_uring</artifactId>
+ <classifier>${netty-transport-native-io_uring-classifier}</classifier>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-transport-classes-io_uring</artifactId>
+ </dependency>
Review Comment:
As I've commented on all the other io_uring PRs, I'd personally prefer folks
had to add the deps if they want them, especially given its off by default, and
its entirely untested.
Or at least for the client, if not both. The way its availability+use is
done would need to change slightly to facilitate it to work without the classes
dep though (elsewhere, I use a class per native type, and thats the only one to
directly reference the types).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
