jbonofre commented on code in PR #2852:
URL: https://github.com/apache/karaf/pull/2852#discussion_r3959909182
##########
diagnostic/boot/src/main/java/org/apache/karaf/diagnostic/core/internal/Activator.java:
##########
@@ -16,28 +16,24 @@
*/
package org.apache.karaf.diagnostic.core.internal;
-import java.io.Closeable;
-
import org.osgi.framework.BundleActivator;
import org.osgi.framework.BundleContext;
public class Activator implements BundleActivator {
- Closeable dumpHandler;
+ DumpHandler dumpHandler;
public void start(BundleContext context) throws Exception {
if (!isWindows()) {
- ClassLoader cl = this.getClass().getClassLoader();
try {
- Class<?> dumpHandlerClazz =
cl.loadClass("org.apache.karaf.diagnostic.core.internal.DumpHandler");
- dumpHandler = (Closeable)
dumpHandlerClazz.getConstructor(BundleContext.class).newInstance(context);
- } catch (Throwable e) {
- // Will happen if sun.misc.SignalHandler is not available
+ dumpHandler = new DumpHandler(context);
+ } catch (Exception e) {
Review Comment:
This narrows the previous `catch (Throwable e)` to `Exception`. The
reflective `DumpHandler` constructor can fail with `Error` subtypes rather than
exceptions (`ExceptionInInitializerError`, `NoClassDefFoundError`,
`LinkageError`). On JVMs where `sun.misc.Signal` resolves as a name but can't
be initialized or linked (GraalVM native image, `-Xrs`, restricted/embedded
JVMs).
Those would now escape `start()` and fail activation of diagnostic bundle,
which is the exact "Signal not available" case this catch is meant to tolerate.
Please keep `catch (Throwable e)` (or `catch Exception | LinkageError`).
##########
diagnostic/boot/src/main/java/org/apache/karaf/diagnostic/core/internal/DumpHandler.java:
##########
@@ -27,20 +29,41 @@
import org.apache.karaf.diagnostic.core.common.ZipDumpDestination;
import org.osgi.framework.BundleContext;
-import sun.misc.Signal;
-import sun.misc.SignalHandler;
+/**
+ * Creates a dump when the process receives SIGHUP. sun.misc.Signal is used
reflectively, as in
+ * org.apache.karaf.main.Main, to avoid the compiler warning about internal
proprietary API.
+ */
+public class DumpHandler implements Closeable {
-public class DumpHandler implements SignalHandler, Closeable {
private static final String SIGNAL = "HUP";
- private BundleContext context;
- private SignalHandler previous;
- public DumpHandler(BundleContext context) {
+ private final BundleContext context;
+ private final Method handleMethod;
+ private final Object signal;
+ private final Object previous;
+
+ public DumpHandler(BundleContext context) throws Exception {
this.context = context;
- previous = sun.misc.Signal.handle(new Signal(SIGNAL), this);
+
+ final Class<?> signalClass = Class.forName("sun.misc.Signal");
+ final Class<?> signalHandlerClass =
Class.forName("sun.misc.SignalHandler");
+
+ Object signalHandler =
Proxy.newProxyInstance(getClass().getClassLoader(),
+ new Class<?>[] {
+ signalHandlerClass
+ },
+ (proxy, method, args) -> {
Review Comment:
The proxy ignores `method`, so every call dispatched to this handler
(including `Object.equals`/`hashCode`/`toString`) triggers a full diagnostic
dump zip in the working directory.
I suggest the following guarding:
```java
(proxy, method, args) -> {
if ("handle".equals(method.getName())) {
handle();
return null;
}
return method.invoke(this, args); // Object methods
}
```
##########
diagnostic/boot/src/main/java/org/apache/karaf/diagnostic/core/internal/DumpHandler.java:
##########
@@ -27,20 +29,41 @@
import org.apache.karaf.diagnostic.core.common.ZipDumpDestination;
import org.osgi.framework.BundleContext;
-import sun.misc.Signal;
-import sun.misc.SignalHandler;
+/**
+ * Creates a dump when the process receives SIGHUP. sun.misc.Signal is used
reflectively, as in
+ * org.apache.karaf.main.Main, to avoid the compiler warning about internal
proprietary API.
+ */
+public class DumpHandler implements Closeable {
-public class DumpHandler implements SignalHandler, Closeable {
private static final String SIGNAL = "HUP";
- private BundleContext context;
- private SignalHandler previous;
- public DumpHandler(BundleContext context) {
+ private final BundleContext context;
+ private final Method handleMethod;
+ private final Object signal;
+ private final Object previous;
+
+ public DumpHandler(BundleContext context) throws Exception {
this.context = context;
- previous = sun.misc.Signal.handle(new Signal(SIGNAL), this);
+
+ final Class<?> signalClass = Class.forName("sun.misc.Signal");
+ final Class<?> signalHandlerClass =
Class.forName("sun.misc.SignalHandler");
+
+ Object signalHandler =
Proxy.newProxyInstance(getClass().getClassLoader(),
+ new Class<?>[] {
+ signalHandlerClass
+ },
+ (proxy, method, args) -> {
+ handle();
+ return null;
+ }
+ );
+
+ handleMethod = signalClass.getMethod("handle", signalClass,
signalHandlerClass);
+ signal = signalClass.getConstructor(String.class).newInstance(SIGNAL);
+ previous = handleMethod.invoke(null, signal, signalHandler);
}
-
- public void handle(Signal signal) {
+
+ private void handle() {
Review Comment:
`Dump.dump` (collect and zip everything) runs synchronously on the JVM
signal-dispatch thread here. `Main.registerSignalHandler` deliberately offloads
its handler body to `new Thread(...)`.
This is pre-existing behaviour, but since the method being rewritten anyway
it would be a good moment to match `Main` and run the dump on a short-lived
thread.
--
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]
