fix(jmx): enable connection to GraalVM native images #222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewazores
changed the title
|
error trace after attempting to view a native image in recording view: |
|
Looks simple enough: |
|
I guess that's not actually it. Tracing the calls: This is in our public synchronized IFlightRecorderService getService()
throws ConnectionException, IOException, ServiceNotAvailableException {
if (!isConnected()) {
connect();
}
IFlightRecorderService service = serviceFactory.getServiceInstance(getHandle());
if (service == null || !isConnected()) {
throw new ConnectionException(
String.format(
"Could not connect to remote target %s",
this.connectionDescriptor.createJMXServiceURL().toString()));
}
return service;
}Here with the @Override
public IFlightRecorderService getServiceInstance(IConnectionHandle handle)
throws ConnectionException, ServiceNotAvailableException {
if (FlightRecorderServiceV2.isAvailable(handle)) {
return new FlightRecorderServiceV2(handle);
}
return new FlightRecorderServiceV1(handle);
}We go into the V2 service constructor and pass in the same non-null reference. public FlightRecorderServiceV2(IConnectionHandle handle) throws ConnectionException, ServiceNotAvailableException {
cfs = handle.getServiceOrThrow(ICommercialFeaturesService.class);
if (!isDynamicFlightRecorderSupported(handle) && isFlightRecorderDisabled(handle)) {
throw new ServiceNotAvailableException(""); //$NON-NLS-1$
}
if (JVMSupportToolkit.isFlightRecorderDisabled(handle, true)) {
throw new ServiceNotAvailableException(""); //$NON-NLS-1$
}
connection = handle;
helper = new FlightRecorderCommunicationHelperV2(handle.getServiceOrThrow(MBeanServerConnection.class));
mbhs = handle.getServiceOrThrow(IMBeanHelperService.class);
serverId = handle.getServerDescriptor().getGUID();
}Here the private boolean isFlightRecorderDisabled(IConnectionHandle handle) {
if (cfs != null && isFlightRecorderCommercial()) {
return !cfs.isCommercialFeaturesEnabled() || JVMSupportToolkit.isFlightRecorderDisabled(handle, false);
} else {
return JVMSupportToolkit.isFlightRecorderDisabled(handle, false);
}
}Again, passing in the non-null reference as private boolean isFlightRecorderCommercial() {
return ConnectionToolkit.isHotSpot(connection)
&& !ConnectionToolkit.isJavaVersionAboveOrEqual(connection, JavaVersionSupport.JFR_NOT_COMMERCIAL);
}This is referencing the |
|
We can patch this since it is occurring in our embedded JMC sources. Worth noting that our embedded JMC sources have diverged from what I see in JMC: vs It looks like our |
|
So, it looks like this ends up being a partial duplicate of #154 / #155 / #95 / #201 . At least some of our embedded JMC sources did not get upgraded from JMC7 to JMC8 equivalents. @Josh-Matsuoka did you come across these classes in the course of #201 ? We should re-update again in this development cycle (sooner rather than later) to make sure all of our forked/embedded JMC sources are from the same version, at least. |
|
Or perhaps these JMX-related classes are ones that we're expecting the JMC Core artifact to extract and distribute so we can consume that as a proper dependency instead of embedded forked sources. I think we still want to upgrade the embedded sources in the meantime regardless. |
diff --git a/src/main/java/org/openjdk/jmc/rjmx/services/ICommercialFeaturesService.java b/src/main/java/org/openjdk/jmc/rjmx/services/ICommercialFeaturesService.java
index e08a443..6a950cc 100644
--- a/src/main/java/org/openjdk/jmc/rjmx/services/ICommercialFeaturesService.java
+++ b/src/main/java/org/openjdk/jmc/rjmx/services/ICommercialFeaturesService.java
@@ -1,6 +1,6 @@
/*
* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- *
+ *
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* The contents of this file are subject to the terms of either the Universal Permissive License
@@ -10,17 +10,17 @@
*
* Redistribution and use in source and binary forms, with or without modification, are permitted
* provided that the following conditions are met:
- *
+ *
* 1. Redistributions of source code must retain the above copyright notice, this list of conditions
* and the following disclaimer.
- *
+ *
* 2. Redistributions in binary form must reproduce the above copyright notice, this list of
* conditions and the following disclaimer in the documentation and/or other materials provided with
* the distribution.
- *
+ *
* 3. Neither the name of the copyright holder nor the names of its contributors may be used to
* endorse or promote products derived from this software without specific prior written permission.
- *
+ *
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
@@ -52,4 +52,6 @@ public interface ICommercialFeaturesService {
*/
void enableCommercialFeatures() throws Exception;
+ boolean hasCommercialFeatures();
+
}
diff --git a/src/main/java/org/openjdk/jmc/rjmx/services/internal/HotSpot23CommercialFeaturesService.java b/src/main/java/org/openjdk/jmc/rjmx/services/internal/HotSpot23CommercialFeaturesService.java
index 94549e3..dccc88f 100644
--- a/src/main/java/org/openjdk/jmc/rjmx/services/internal/HotSpot23CommercialFeaturesService.java
+++ b/src/main/java/org/openjdk/jmc/rjmx/services/internal/HotSpot23CommercialFeaturesService.java
@@ -1,6 +1,6 @@
/*
* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- *
+ *
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* The contents of this file are subject to the terms of either the Universal Permissive License
@@ -10,17 +10,17 @@
*
* Redistribution and use in source and binary forms, with or without modification, are permitted
* provided that the following conditions are met:
- *
+ *
* 1. Redistributions of source code must retain the above copyright notice, this list of conditions
* and the following disclaimer.
- *
+ *
* 2. Redistributions in binary form must reproduce the above copyright notice, this list of
* conditions and the following disclaimer in the documentation and/or other materials provided with
* the distribution.
- *
+ *
* 3. Neither the name of the copyright holder nor the names of its contributors may be used to
* endorse or promote products derived from this software without specific prior written permission.
- *
+ *
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
@@ -96,4 +96,9 @@ public class HotSpot23CommercialFeaturesService implements ICommercialFeaturesSe
server.getMBeanInfo(candidateObjectName);
return candidateObjectName;
}
+
+ @Override
+ public boolean hasCommercialFeatures() {
+ return false;
+ }
}
diff --git a/src/main/java/org/openjdk/jmc/rjmx/services/internal/Jdk11CommercialFeaturesService.java b/src/main/java/org/openjdk/jmc/rjmx/services/internal/Jdk11CommercialFeaturesService.java
index 8453526..1ec3d21 100644
--- a/src/main/java/org/openjdk/jmc/rjmx/services/internal/Jdk11CommercialFeaturesService.java
+++ b/src/main/java/org/openjdk/jmc/rjmx/services/internal/Jdk11CommercialFeaturesService.java
@@ -1,6 +1,6 @@
/*
* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- *
+ *
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* The contents of this file are subject to the terms of either the Universal Permissive License
@@ -10,17 +10,17 @@
*
* Redistribution and use in source and binary forms, with or without modification, are permitted
* provided that the following conditions are met:
- *
+ *
* 1. Redistributions of source code must retain the above copyright notice, this list of conditions
* and the following disclaimer.
- *
+ *
* 2. Redistributions in binary form must reproduce the above copyright notice, this list of
* conditions and the following disclaimer in the documentation and/or other materials provided with
* the distribution.
- *
+ *
* 3. Neither the name of the copyright holder nor the names of its contributors may be used to
* endorse or promote products derived from this software without specific prior written permission.
- *
+ *
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
@@ -35,7 +35,7 @@ package org.openjdk.jmc.rjmx.services.internal;
import org.openjdk.jmc.rjmx.services.ICommercialFeaturesService;
public class Jdk11CommercialFeaturesService implements ICommercialFeaturesService {
-
+
@Override
public boolean isCommercialFeaturesEnabled() {
return true;
@@ -45,4 +45,9 @@ public class Jdk11CommercialFeaturesService implements ICommercialFeaturesServic
public void enableCommercialFeatures() throws Exception {
// Noop
}
+
+ @Override
+ public boolean hasCommercialFeatures() {
+ return false;
+ }
}
diff --git a/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/FlightRecorderServiceV2.java b/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/FlightRecorderServiceV2.java
index dd1e423..bcd09f3 100644
--- a/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/FlightRecorderServiceV2.java
+++ b/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/FlightRecorderServiceV2.java
@@ -1,6 +1,6 @@
/*
- * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- *
+ * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
+ *
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* The contents of this file are subject to the terms of either the Universal Permissive License
@@ -10,17 +10,17 @@
*
* Redistribution and use in source and binary forms, with or without modification, are permitted
* provided that the following conditions are met:
- *
+ *
* 1. Redistributions of source code must retain the above copyright notice, this list of conditions
* and the following disclaimer.
- *
+ *
* 2. Redistributions in binary form must reproduce the above copyright notice, this list of
* conditions and the following disclaimer in the documentation and/or other materials provided with
* the distribution.
- *
+ *
* 3. Neither the name of the copyright holder nor the names of its contributors may be used to
* endorse or promote products derived from this software without specific prior written permission.
- *
+ *
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
@@ -103,17 +103,14 @@ public class FlightRecorderServiceV2 implements IFlightRecorderService {
}
private boolean isDynamicFlightRecorderSupported(IConnectionHandle handle) {
- return ConnectionToolkit.isHotSpot(handle)
- && ConnectionToolkit.isJavaVersionAboveOrEqual(handle, JavaVersionSupport.DYNAMIC_JFR_SUPPORTED);
- }
-
- private boolean isFlightRecorderCommercial() {
- return ConnectionToolkit.isHotSpot(connection)
- && !ConnectionToolkit.isJavaVersionAboveOrEqual(connection, JavaVersionSupport.JFR_NOT_COMMERCIAL);
+ // All OpenJDK versions of JFR support dynamic enablement of JFR, so if there are no commercial features in play
+ // all is A-OK.
+ return !cfs.hasCommercialFeatures() || (ConnectionToolkit.isHotSpot(handle)
+ && ConnectionToolkit.isJavaVersionAboveOrEqual(handle, JavaVersionSupport.DYNAMIC_JFR_SUPPORTED));
}
private boolean isFlightRecorderDisabled(IConnectionHandle handle) {
- if (cfs != null && isFlightRecorderCommercial()) {
+ if (cfs != null && cfs.hasCommercialFeatures()) {
return !cfs.isCommercialFeaturesEnabled() || JVMSupportToolkit.isFlightRecorderDisabled(handle, false);
} else {
return JVMSupportToolkit.isFlightRecorderDisabled(handle, false);
@@ -126,6 +123,7 @@ public class FlightRecorderServiceV2 implements IFlightRecorderService {
public FlightRecorderServiceV2(IConnectionHandle handle) throws ConnectionException, ServiceNotAvailableException {
cfs = handle.getServiceOrThrow(ICommercialFeaturesService.class);
+
if (!isDynamicFlightRecorderSupported(handle) && isFlightRecorderDisabled(handle)) {
throw new ServiceNotAvailableException(""); //$NON-NLS-1$
}
@@ -418,8 +416,7 @@ public class FlightRecorderServiceV2 implements IFlightRecorderService {
Long id = (Long) helper.invokeOperation("cloneRecording", //$NON-NLS-1$
descriptor.getId(), Boolean.TRUE);
IMutableConstrainedMap<String> options = getEmptyRecordingOptions();
- options.put(RecordingOptionsBuilder.KEY_NAME,
- String.format("Clone of %s", descriptor.getName()));
+ options.put(RecordingOptionsBuilder.KEY_NAME, String.format("Clone of %s", descriptor.getName()));
helper.invokeOperation("setRecordingOptions", id, toTabularData(options)); //$NON-NLS-1$
return getUpdatedRecordingDescriptor(id);
} catch (Exception e) {
@@ -480,7 +477,7 @@ public class FlightRecorderServiceV2 implements IFlightRecorderService {
@Override
public boolean isEnabled() {
if (!wasEnabled) {
- boolean isEnabled = isFlightRecorderCommercial() ? cfs.isCommercialFeaturesEnabled()
+ boolean isEnabled = cfs.hasCommercialFeatures() ? cfs.isCommercialFeaturesEnabled()
: isAvailable(connection);
if (isEnabled) {
wasEnabled = true;
@@ -500,3 +497,4 @@ public class FlightRecorderServiceV2 implements IFlightRecorderService {
}
}
}
+I prepared this patch by simply copy-pasting the JMC8 version of the After a rebuild:
I appear to be able to connect to @roberttoyonaga 's native image application and read some basic JFR information. |
mwangggg
force-pushed
the
217-NPE-when-connecting-native-image
branch
from
3fc20a3 to
f61cd54
Compare
andrewazores
previously approved these changes
May 26, 2023
There was a problem hiding this comment.
Change looks good, but we should wait for #223 before merging this.
This was referenced May 31, 2023
andrewazores
force-pushed
the
217-NPE-when-connecting-native-image
branch
from
f61cd54 to
9f987c3
Compare
andrewazores
approved these changes
Jun 7, 2023
mergify bot
pushed a commit
that referenced
this pull request
Jun 13, 2023
(cherry picked from commit 752b986)
andrewazores
pushed a commit
that referenced
this pull request
Jun 13, 2023
(cherry picked from commit 752b986)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #217
Depends on #223