diff --git a/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java b/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java index c958856d2..3b76bdf3e 100644 --- a/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java +++ b/btrace-agent/src/main/java/io/btrace/agent/ClientContext.java @@ -34,6 +34,15 @@ class ClientContext { ClientContext( Instrumentation instr, BTraceTransformer transformer, ArgsMap args, SharedSettings settings) { + // A client's SET_PARAMS command mutates these settings in place, so a context must never be + // built on the agent-wide instance: that would let one client's debug/dumpDir/outputFile, + // granted permissions and non-downgradable `trusted` flag leak into every other client and + // into the global transformer. Seed a per-client copy with Main#newClientSettings instead. + if (settings == SharedSettings.GLOBAL) { + throw new IllegalArgumentException( + "ClientContext must not share SharedSettings.GLOBAL; use Main#newClientSettings(..)" + + " to seed an isolated per-client copy"); + } this.instr = instr; this.transformer = transformer; this.args = args; diff --git a/btrace-agent/src/main/java/io/btrace/agent/Main.java b/btrace-agent/src/main/java/io/btrace/agent/Main.java index 7e546bddc..1cd3df3f4 100644 --- a/btrace-agent/src/main/java/io/btrace/agent/Main.java +++ b/btrace-agent/src/main/java/io/btrace/agent/Main.java @@ -180,6 +180,27 @@ public final class Main { private static final SharedSettings settings = SharedSettings.GLOBAL; private static final BTraceTransformer transformer = new BTraceTransformer(new DebugSupport(settings)); + // ClassFile API's StackMapGenerator can exhaust the default stack depth when processing + // complex methods in JDK 26+ (class-file major >= 70). Use 4 MB on those JVMs only; on + // older JVMs keep the default (0 = JVM-chosen) to avoid any OS-level stack-allocation + // overhead that was intermittently delaying test startup on JDK 8/11/21 CI machines. + static final long QUEUE_PROCESSOR_STACK_SIZE = resolveQueueProcessorStackSize(); + + static long resolveQueueProcessorStackSize() { + return resolveQueueProcessorStackSize(System.getProperty("java.class.version", "52.0")); + } + + // Visible for testing. + static long resolveQueueProcessorStackSize(String classVersion) { + try { + int dot = classVersion.indexOf('.'); + int major = Integer.parseInt(dot >= 0 ? classVersion.substring(0, dot) : classVersion); + return major >= 70 ? 4L * 1024 * 1024 : 0L; + } catch (RuntimeException ignored) { + return 0L; + } + } + // #BTRACE-42: Non-daemon thread prevents traced application from exiting // Deliberately an anonymous class, not a lambda: this static field initializer runs in // Main's , unconditionally, before -javaagent premain()'s body even starts -- see @@ -188,7 +209,8 @@ public final class Main { new ThreadFactory() { @Override public Thread newThread(Runnable r) { - Thread result = new Thread(r, "BTrace Command Queue Processor"); + Thread result = + new Thread(null, r, "BTrace Command Queue Processor", QUEUE_PROCESSOR_STACK_SIZE); result.setDaemon(true); return result; } @@ -1457,8 +1479,7 @@ private static boolean loadBTraceScript(String filePath, boolean traceToStdOut) return false; } - SharedSettings clientSettings = new SharedSettings(); - clientSettings.from(settings); + SharedSettings clientSettings = newClientSettings(settings); clientSettings.setClientName(scriptName); if (traceToStdOut) { clientSettings.setOutputFile("::stdout"); @@ -1511,7 +1532,7 @@ private static void runServer(ControlServer endpoint) { log.debug("client accepted from {}", sock.getRemoteSocketAddress()); } authenticationToken = endpoint.copyAuthenticationToken(); - ClientContext ctx = new ClientContext(inst, transformer, argMap, settings); + ClientContext ctx = newRemoteClientContext(); Client client = RemoteClient.getClient( ctx, @@ -1569,6 +1590,39 @@ private static synchronized void shutdownServer() { } } + /** + * Seeds a fresh per-client settings instance from the agent baseline. + * + *

Every client - local and remote alike - must get its own copy. A client's {@code SET_PARAMS} + * command is applied by mutating its {@link ClientContext}'s settings in place, so handing out + * the shared {@link SharedSettings#GLOBAL} instance lets one client's {@code debug}, {@code + * dumpDir}, {@code outputFile}, granted permissions and - most importantly - {@code trusted} flag + * leak into every other client and into the global transformer. {@code trusted} is deliberately + * non-downgrading, so such a leak is permanent for the lifetime of the agent. + * + * @param baseline the agent-wide settings to seed from + * @return a new settings instance, independent of {@code baseline} + */ + static SharedSettings newClientSettings(SharedSettings baseline) { + SharedSettings clientSettings = new SharedSettings(); + clientSettings.from(baseline); + return clientSettings; + } + + /** + * Builds the context for a newly accepted remote connection. + * + *

Extracted from the accept loop so the settings isolation it depends on is reachable from a + * unit test: inside the loop it sits under a {@code catch (RuntimeException | IOException)} that + * would turn a re-introduced sharing bug into a per-connection warning rather than a test + * failure. + * + * @return a context carrying settings private to this connection + */ + static ClientContext newRemoteClientContext() { + return new ClientContext(inst, transformer, argMap, newClientSettings(settings)); + } + private static Future handleNewClient(Client client) { // Deliberately an anonymous class, not a lambda: this method can run reachable from // -javaagent premain(), before the JVM's own java.lang.invoke bootstrap is guaranteed diff --git a/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java b/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java new file mode 100644 index 000000000..32d4c1e6c --- /dev/null +++ b/btrace-agent/src/test/java/io/btrace/agent/ClientSettingsIsolationTest.java @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed 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 + * + * https://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 io.btrace.agent; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.btrace.core.ArgsMap; +import io.btrace.core.SharedSettings; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Every client must trace against its own {@link SharedSettings} copy. + * + *

A client's {@code SET_PARAMS} command is applied by {@code RemoteClient} as {@code + * ctx.getSettings().from(params)} - an in-place mutation. If contexts share the agent-wide + * instance, one client's parameters silently become every client's parameters. {@code trusted} is + * the dangerous case: {@link SharedSettings#from(Map)} ORs it in and never clears it, so a single + * client asking for {@code trusted} would permanently elevate the whole agent. + * + *

This isolation has been implemented and then accidentally reverted once already, with nothing + * failing. These tests exist so the next revert breaks the build. + */ +class ClientSettingsIsolationTest { + + @Test + @DisplayName("per-client settings are a copy, not the agent-wide instance") + void clientSettingsAreDistinctFromBaseline() { + SharedSettings baseline = new SharedSettings(); + SharedSettings clientSettings = Main.newClientSettings(baseline); + + assertNotSame(baseline, clientSettings, "client settings must not alias the baseline"); + assertNotSame( + SharedSettings.GLOBAL, + clientSettings, + "client settings must not alias the global instance"); + } + + @Test + @DisplayName("per-client settings inherit the agent baseline") + void clientSettingsInheritBaseline() { + SharedSettings baseline = new SharedSettings(); + baseline.setDebug(true); + baseline.setOutputFile("agent-baseline.btrace"); + + SharedSettings clientSettings = Main.newClientSettings(baseline); + + assertTrue(clientSettings.isDebug(), "baseline debug flag should be inherited"); + assertEquals("agent-baseline.btrace", clientSettings.getOutputFile()); + } + + @Test + @DisplayName("one client's SET_PARAMS does not leak into the agent baseline") + void setParamsDoesNotLeakToBaseline() { + SharedSettings baseline = new SharedSettings(); + SharedSettings clientSettings = Main.newClientSettings(baseline); + + // Mirrors RemoteClient's handling of Command.SET_PARAMS. + clientSettings.from(setParams()); + + assertTrue(clientSettings.isDebug(), "the requesting client should see its own parameters"); + assertFalse(baseline.isDebug(), "debug leaked into the agent baseline"); + assertFalse(baseline.isTrusted(), "trusted leaked into the agent baseline"); + assertEquals(null, baseline.getDumpDir(), "dumpDir leaked into the agent baseline"); + } + + @Test + @DisplayName("one client's SET_PARAMS does not leak into another client") + void setParamsDoesNotLeakBetweenClients() { + SharedSettings baseline = new SharedSettings(); + SharedSettings first = Main.newClientSettings(baseline); + SharedSettings second = Main.newClientSettings(baseline); + + first.from(setParams()); + + assertTrue(first.isTrusted(), "the requesting client should see its own parameters"); + assertFalse(second.isTrusted(), "trusted leaked into a concurrently connected client"); + assertFalse(second.isDebug(), "debug leaked into a concurrently connected client"); + } + + @Test + @DisplayName("trusted cannot be downgraded, so a leak would be permanent") + void trustedIsNonDowngrading() { + SharedSettings settings = new SharedSettings(); + settings.from(setParams()); + assertTrue(settings.isTrusted()); + + Map untrust = new HashMap<>(); + untrust.put(SharedSettings.TRUSTED_KEY, Boolean.FALSE); + settings.from(untrust); + + assertTrue( + settings.isTrusted(), + "trusted is deliberately non-downgrading - which is exactly why sharing the instance" + + " between clients is unsafe"); + } + + @Test + @DisplayName("a ClientContext cannot be built on the agent-wide settings") + void clientContextRejectsGlobalSettings() { + IllegalArgumentException failure = + assertThrows( + IllegalArgumentException.class, + () -> new ClientContext(null, null, new ArgsMap(), SharedSettings.GLOBAL)); + + assertTrue( + failure.getMessage().contains("newClientSettings"), + "the failure should point at the supported way to seed per-client settings"); + } + + @Test + @DisplayName("a ClientContext accepts an isolated copy") + void clientContextAcceptsIsolatedSettings() { + ClientContext ctx = + new ClientContext(null, null, new ArgsMap(), Main.newClientSettings(SharedSettings.GLOBAL)); + + assertNotSame(SharedSettings.GLOBAL, ctx.getSettings()); + } + + @Test + @DisplayName("the remote accept path builds contexts on isolated settings") + void remoteContextDoesNotShareGlobalSettings() { + ClientContext ctx = Main.newRemoteClientContext(); + + assertNotSame( + SharedSettings.GLOBAL, + ctx.getSettings(), + "the remote accept path handed out the agent-wide settings instance"); + } + + @Test + @DisplayName("two remote connections do not share settings with each other") + void remoteContextsAreIndependent() { + ClientContext first = Main.newRemoteClientContext(); + ClientContext second = Main.newRemoteClientContext(); + + assertNotSame(first.getSettings(), second.getSettings()); + + first.getSettings().from(setParams()); + + assertFalse(second.getSettings().isTrusted(), "trusted leaked between remote connections"); + assertFalse(SharedSettings.GLOBAL.isTrusted(), "trusted leaked into the agent-wide settings"); + } + + private static Map setParams() { + Map params = new HashMap<>(); + params.put(SharedSettings.DEBUG_KEY, Boolean.TRUE); + params.put(SharedSettings.TRUSTED_KEY, Boolean.TRUE); + params.put(SharedSettings.DUMP_DIR_KEY, "/tmp/leaked-dump-dir"); + return params; + } +} diff --git a/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java b/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java new file mode 100644 index 000000000..09799c2e1 --- /dev/null +++ b/btrace-agent/src/test/java/io/btrace/agent/QueueProcessorStackSizeTest.java @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed 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 + * + * https://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 io.btrace.agent; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * The command-queue processor thread needs an enlarged stack on JDK 26+. + * + *

ClassFile API's {@code StackMapGenerator} can exhaust the default stack depth while processing + * complex methods on class-file major 70 and above. Older JVMs keep the JVM-chosen default, because + * an explicit stack size there was intermittently delaying test startup on JDK 8/11/21 CI machines. + * + *

This sizing has been dropped by an accidental revert once already; these tests pin both halves + * of the trade-off. + */ +class QueueProcessorStackSizeTest { + + private static final long FOUR_MB = 4L * 1024 * 1024; + + @Test + @DisplayName("JDK 26+ class files get an enlarged stack") + void enlargedStackOnJdk26AndLater() { + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("70.0"), "JDK 26 (major 70)"); + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("71.0"), "JDK 27 (major 71)"); + assertEquals(FOUR_MB, Main.resolveQueueProcessorStackSize("99"), "far-future JVM"); + } + + @Test + @DisplayName("older JVMs keep the JVM-chosen default") + void defaultStackBelowJdk26() { + assertEquals(0L, Main.resolveQueueProcessorStackSize("52.0"), "JDK 8 (major 52)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("55.0"), "JDK 11 (major 55)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("65.0"), "JDK 21 (major 65)"); + assertEquals(0L, Main.resolveQueueProcessorStackSize("69.0"), "JDK 25 (major 69)"); + } + + @Test + @DisplayName("an unparseable class version falls back to the default") + void malformedClassVersionFallsBack() { + assertEquals(0L, Main.resolveQueueProcessorStackSize("")); + assertEquals(0L, Main.resolveQueueProcessorStackSize("not-a-version")); + assertEquals(0L, Main.resolveQueueProcessorStackSize(".7")); + } + + @Test + @DisplayName("the constant is wired to the running JVM's class version") + void constantMatchesRunningJvm() { + assertEquals( + Main.resolveQueueProcessorStackSize(System.getProperty("java.class.version", "52.0")), + Main.QUEUE_PROCESSOR_STACK_SIZE, + "QUEUE_PROCESSOR_STACK_SIZE must be derived from java.class.version"); + } +} diff --git a/btrace-client/src/main/java/io/btrace/client/Client.java b/btrace-client/src/main/java/io/btrace/client/Client.java index 90a52b789..0132d08c8 100644 --- a/btrace-client/src/main/java/io/btrace/client/Client.java +++ b/btrace-client/src/main/java/io/btrace/client/Client.java @@ -23,6 +23,7 @@ import io.btrace.compiler.Compiler; import io.btrace.core.Args; import io.btrace.core.BTraceRuntime; +import io.btrace.core.JavaVersionCheck; import io.btrace.core.SharedSettings; import io.btrace.core.annotations.DTrace; import io.btrace.core.annotations.DTraceRef; @@ -211,21 +212,21 @@ public Client( */ private static void warnIfDeprecatedTargetJvm(String pid, String targetJavaVersion) { try { - int featureVersion = io.btrace.core.JavaVersionCheck.parseFeatureVersion(targetJavaVersion); - if (io.btrace.core.JavaVersionCheck.isDeprecated(featureVersion) - && !Boolean.getBoolean(io.btrace.core.JavaVersionCheck.SUPPRESS_PROP)) { + int featureVersion = JavaVersionCheck.parseFeatureVersion(targetJavaVersion); + if (JavaVersionCheck.isDeprecated(featureVersion) + && !Boolean.getBoolean(JavaVersionCheck.SUPPRESS_PROP)) { System.err.println( "[BTrace] WARNING: The target JVM (PID " + pid + ") is Java " + featureVersion + ". Running BTrace on Java versions older than " - + io.btrace.core.JavaVersionCheck.DEPRECATION_FLOOR + + JavaVersionCheck.DEPRECATION_FLOOR + " is deprecated and support will be removed in the next major release. " + "Please upgrade the target JVM to Java " - + io.btrace.core.JavaVersionCheck.DEPRECATION_FLOOR + + JavaVersionCheck.DEPRECATION_FLOOR + " or newer. Suppress this warning with -D" - + io.btrace.core.JavaVersionCheck.SUPPRESS_PROP + + JavaVersionCheck.SUPPRESS_PROP + "=true."); } } catch (Throwable ignored) { @@ -233,6 +234,28 @@ private static void warnIfDeprecatedTargetJvm(String pid, String targetJavaVersi } } + /** + * Appends a single {@code key=value} entry to a comma-separated agent argument string. + * + *

The agent splits each entry on its first {@code =} and matches on the key, so an entry whose + * key is empty or misplaced is not rejected - it is silently ignored, and the setting it carried + * is lost with no diagnostic. A hand-rolled {@code ",=" + KEY + value} typo did exactly that to + * {@code cmdQueueLimit} for two releases. Routing every entry through here keeps the shape + * uniform and lets {@code ClientAgentArgsTest} assert it. + * + * @param agentArgs the arguments accumulated so far; empty for the first entry + * @param key the argument name, which must be non-empty and free of {@code =} and {@code ,} + * @param value the argument value, rendered with {@link String#valueOf(Object)} + * @return {@code agentArgs} with the new entry appended + */ + static String appendAgentArg(String agentArgs, String key, Object value) { + if (key == null || key.isEmpty() || key.indexOf('=') >= 0 || key.indexOf(',') >= 0) { + throw new IllegalArgumentException("invalid agent argument key: " + key); + } + String entry = key + "=" + String.valueOf(value); + return agentArgs.isEmpty() ? entry : agentArgs + "," + entry; + } + private static boolean isPortAvailable(int port) { Socket clSocket = null; try { @@ -772,25 +795,25 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th log.debug("attached to {}", pid); log.debug("loading {}", agentPath); } - String agentArgs = Args.PORT + "=" + port; + String agentArgs = appendAgentArg("", Args.PORT, port); if (statsdDef != null) { - agentArgs += "," + Args.STATSD + "=" + statsdDef; + agentArgs = appendAgentArg(agentArgs, Args.STATSD, statsdDef); } if (debug) { - agentArgs += "," + Args.DEBUG + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.DEBUG, "true"); } if (trusted) { - agentArgs += "," + Args.TRUSTED + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.TRUSTED, "true"); } if (dumpClasses) { - agentArgs += "," + Args.DUMP_CLASSES + "=true"; - agentArgs += "," + Args.DUMP_DIR + "=" + dumpDir; + agentArgs = appendAgentArg(agentArgs, Args.DUMP_CLASSES, "true"); + agentArgs = appendAgentArg(agentArgs, Args.DUMP_DIR, dumpDir); } if (trackRetransforms) { - agentArgs += "," + Args.TRACK_RETRANSFORMS + "=true"; + agentArgs = appendAgentArg(agentArgs, Args.TRACK_RETRANSFORMS, "true"); } if (bootCp != null) { - agentArgs += "," + Args.BOOT_CLASS_PATH + "=" + bootCp; + agentArgs = appendAgentArg(agentArgs, Args.BOOT_CLASS_PATH, bootCp); } String toolsPath = getToolsJarPath( @@ -800,12 +823,12 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th } else { sysCp = sysCp + File.pathSeparator + toolsPath; } - agentArgs += "," + Args.SYSTEM_CLASS_PATH + "=" + sysCp; + agentArgs = appendAgentArg(agentArgs, Args.SYSTEM_CLASS_PATH, sysCp); String cmdQueueLimit = System.getProperty(BTraceRuntime.CMD_QUEUE_LIMIT_KEY, null); if (cmdQueueLimit != null) { - agentArgs += "," + Args.CMD_QUEUE_LIMIT + "=" + cmdQueueLimit; + agentArgs = appendAgentArg(agentArgs, Args.CMD_QUEUE_LIMIT, cmdQueueLimit); } - agentArgs += "," + Args.PROBE_DESC_PATH + "=" + probeDescPath; + agentArgs = appendAgentArg(agentArgs, Args.PROBE_DESC_PATH, probeDescPath); // Pass-through selected system properties as agent system props via "$" args // so the agent can read them at startup. These become system properties in the target JVM. @@ -818,28 +841,28 @@ public void attach(String pid, String agentPath, String sysCp, String bootCp) th throw new IllegalArgumentException( "System property btrace.feature.manifestLibs must not contain ','"); } - agentArgs += "," + "$btrace.feature.manifestLibs" + "=" + manifestLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.feature.manifestLibs", manifestLibs); } if (sysAppendJar != null && !sysAppendJar.isEmpty()) { if (sysAppendJar.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.system.appendJar must not contain ','"); } - agentArgs += "," + "$btrace.system.appendJar" + "=" + sysAppendJar; + agentArgs = appendAgentArg(agentArgs, "$btrace.system.appendJar", sysAppendJar); } if (allowExternalLibs != null && !allowExternalLibs.isEmpty()) { if (allowExternalLibs.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.allowExternalLibs must not contain ','"); } - agentArgs += "," + "$btrace.allowExternalLibs" + "=" + allowExternalLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.allowExternalLibs", allowExternalLibs); } if (testSkipLibs != null && !testSkipLibs.isEmpty()) { if (testSkipLibs.indexOf(',') >= 0) { throw new IllegalArgumentException( "System property btrace.test.skipLibs must not contain ','"); } - agentArgs += "," + "$btrace.test.skipLibs" + "=" + testSkipLibs; + agentArgs = appendAgentArg(agentArgs, "$btrace.test.skipLibs", testSkipLibs); } if (log.isDebugEnabled()) { log.debug("agent args: {}", agentArgs); diff --git a/btrace-client/src/main/java/io/btrace/client/Main.java b/btrace-client/src/main/java/io/btrace/client/Main.java index a4a661077..c008aad5f 100644 --- a/btrace-client/src/main/java/io/btrace/client/Main.java +++ b/btrace-client/src/main/java/io/btrace/client/Main.java @@ -22,6 +22,7 @@ import io.btrace.compiler.oneliner.OnelinerParser; import io.btrace.compiler.oneliner.OnelinerValidator; import io.btrace.core.DebugSupport; +import io.btrace.core.JavaVersionCheck; import io.btrace.core.Messages; import io.btrace.core.comm.Command; import io.btrace.core.comm.CommandListener; @@ -32,9 +33,6 @@ import java.io.File; import java.io.IOException; import java.io.PrintWriter; -import java.nio.file.Files; -import java.nio.file.Paths; -import java.util.Properties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sun.misc.Signal; @@ -84,28 +82,23 @@ public final class Main { if (log.isDebugEnabled()) log.debug("dumpDir is {}", DUMP_DIR); } PROBE_DESC_PATH = System.getProperty("com.sun.btrace.probeDescPath", "."); - String javaVersion = getJavaVersion(); - // In Java 22 the console will write standard output to stderr :shrug: - con = javaVersion == null || !javaVersion.startsWith("22") ? System.console() : null; + con = suppressConsole(JavaVersionCheck.javaFeatureVersion()) ? null : System.console(); out = getOutWriter(con); } - private static String getJavaVersion() { - String javaHome = System.getenv("JAVA_HOME"); - if (javaHome == null || javaHome.trim().isEmpty()) { - // Fall back to the JVM the client itself is running on; JAVA_HOME is not required to be set. - javaHome = System.getProperty("java.home"); - } - if (javaHome == null || javaHome.trim().isEmpty()) { - return null; - } - Properties props = new Properties(); - try { - props.load(Files.newInputStream(Paths.get(javaHome, "release"))); - return props.getProperty("JAVA_VERSION").replace("\"", ""); - } catch (IOException ignored) { - return null; - } + /** + * Whether {@link System#console()} must be bypassed on the given Java feature version. + * + *

Java 22's console writes standard output to stderr, so the client falls back to {@link + * System#out} there. The version that matters is the one of the JVM running this client - not + * {@code JAVA_HOME}, which need not point at it - so this is decided from {@code + * java.specification.version} rather than by reading a {@code release} file off disk. + * + * @param featureVersion a Java feature version, or {@code -1} when it can not be determined + * @return {@code true} to bypass the console + */ + static boolean suppressConsole(int featureVersion) { + return featureVersion == 22; } @SuppressWarnings("DefaultCharset") diff --git a/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java b/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java new file mode 100644 index 000000000..d010fddc0 --- /dev/null +++ b/btrace-client/src/test/java/io/btrace/client/ClientAgentArgsTest.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed 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 + * + * https://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 io.btrace.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.btrace.core.Args; +import io.btrace.core.ArgsMap; +import java.util.Map; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * The agent argument string the client hands to {@code VirtualMachine#loadAgent} must survive the + * agent's own parsing. + * + *

{@link ArgsMap} splits each comma-separated entry on {@code =} and dispatches on the key, so a + * malformed entry is not an error - it parses into a key nothing matches and the setting is dropped + * in silence. {@code cmdQueueLimit} was shipped that way, fixed, then reverted back by an unrelated + * commit, because nothing asserted the wire shape. These tests assert it. + */ +class ClientAgentArgsTest { + + @Test + @DisplayName("the first entry carries no leading separator") + void firstEntryHasNoLeadingComma() { + assertEquals("port=2020", Client.appendAgentArg("", Args.PORT, 2020)); + } + + @Test + @DisplayName("subsequent entries are comma separated") + void subsequentEntriesAreCommaSeparated() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.DEBUG, "true"); + + assertEquals("port=2020,debug=true", args); + } + + @Test + @DisplayName("cmdQueueLimit round-trips through the agent's parser") + void cmdQueueLimitRoundTripsThroughArgsMap() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.CMD_QUEUE_LIMIT, "512"); + + ArgsMap parsed = new ArgsMap(args.split(",")); + + assertEquals( + "512", + parsed.get(Args.CMD_QUEUE_LIMIT), + "the agent must see cmdQueueLimit as a key, not as part of a value"); + } + + @Test + @DisplayName("a fully populated argument string parses without an empty key") + void noEntryParsesToAnEmptyKey() { + String args = Client.appendAgentArg("", Args.PORT, 2020); + args = Client.appendAgentArg(args, Args.STATSD, "localhost:8125"); + args = Client.appendAgentArg(args, Args.DEBUG, "true"); + args = Client.appendAgentArg(args, Args.TRUSTED, "true"); + args = Client.appendAgentArg(args, Args.DUMP_CLASSES, "true"); + args = Client.appendAgentArg(args, Args.DUMP_DIR, "/tmp/dump"); + args = Client.appendAgentArg(args, Args.TRACK_RETRANSFORMS, "true"); + args = Client.appendAgentArg(args, Args.CMD_QUEUE_LIMIT, "512"); + args = Client.appendAgentArg(args, Args.PROBE_DESC_PATH, "."); + args = Client.appendAgentArg(args, "$btrace.system.appendJar", "/tmp/x.jar"); + + ArgsMap parsed = new ArgsMap(args.split(",")); + + for (Map.Entry entry : parsed) { + assertFalse( + entry.getKey().isEmpty(), + "an empty key means an entry was built with a misplaced '='; the agent silently drops it"); + } + assertEquals("512", parsed.get(Args.CMD_QUEUE_LIMIT)); + assertEquals("localhost:8125", parsed.get(Args.STATSD)); + assertEquals("/tmp/x.jar", parsed.get("$btrace.system.appendJar")); + } + + @Test + @DisplayName("a malformed key is rejected rather than silently dropped downstream") + void malformedKeysAreRejected() { + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "", "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", null, "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "=bad", "v")); + assertThrows(IllegalArgumentException.class, () -> Client.appendAgentArg("", "ba,d", "v")); + } + + /** + * Pins the failure mode the guard exists for: the historical {@code ",=" + KEY + value} typo + * parses into an empty key, which no {@code case} in the agent matches. + */ + @Test + @DisplayName("the historical malformed shape does parse to an empty key") + void malformedShapeParsesToEmptyKey() { + String malformed = "port=2020" + ",=" + Args.CMD_QUEUE_LIMIT + "512"; + + ArgsMap parsed = new ArgsMap(malformed.split(",")); + + assertEquals(null, parsed.get(Args.CMD_QUEUE_LIMIT), "the setting is lost, not reported"); + assertEquals(Args.CMD_QUEUE_LIMIT + "512", parsed.get(""), "it lands under an empty key"); + } +} diff --git a/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java b/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java new file mode 100644 index 000000000..031b06efb --- /dev/null +++ b/btrace-client/src/test/java/io/btrace/client/ConsoleSelectionTest.java @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed 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 + * + * https://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 io.btrace.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.btrace.core.JavaVersionCheck; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * The client bypasses {@link System#console()} on Java 22 only, because that release writes + * standard output to stderr. + * + *

The decision must follow the JVM actually running the client. It used to be derived from a + * {@code release} file located through {@code JAVA_HOME}, which is not required to be set, need not + * point at the running JVM, and cost a file read plus two unchecked-exception surfaces inside a + * static initializer - where any throw becomes an {@code ExceptionInInitializerError} that stops + * the CLI from starting at all. + */ +class ConsoleSelectionTest { + + @Test + @DisplayName("Java 22 bypasses the console") + void java22BypassesConsole() { + assertTrue(Main.suppressConsole(22)); + } + + @ParameterizedTest(name = "Java {0} uses the console") + @ValueSource(ints = {8, 11, 17, 21, 23, 24, 25, 26, 27}) + @DisplayName("every other release uses the console") + void otherVersionsUseConsole(int featureVersion) { + assertFalse( + Main.suppressConsole(featureVersion), + "only Java 22 has the stderr redirection; " + featureVersion + " must keep the console"); + } + + @Test + @DisplayName("an undeterminable version falls back to using the console") + void unknownVersionUsesConsole() { + assertFalse( + Main.suppressConsole(-1), + "an unknown version must behave as before: use the console rather than guess"); + } + + @Test + @DisplayName("the running JVM's version is resolved without JAVA_HOME") + void runningVersionResolvesIndependentlyOfJavaHome() { + // The suite runs both with and without JAVA_HOME set; neither may change the answer, and + // neither may leave the version undeterminable. + int featureVersion = JavaVersionCheck.javaFeatureVersion(); + + assertTrue( + featureVersion >= 8, + "the running JVM's feature version must be resolvable from system properties alone," + + " independently of JAVA_HOME; got " + + featureVersion); + assertEquals( + JavaVersionCheck.parseFeatureVersion(System.getProperty("java.specification.version")), + featureVersion, + "the version must come from the running JVM, not from an external installation"); + } +} diff --git a/btrace-dist/build.gradle b/btrace-dist/build.gradle index 67fd1d35b..e2b7aa5b0 100644 --- a/btrace-dist/build.gradle +++ b/btrace-dist/build.gradle @@ -509,9 +509,14 @@ task copyDtraceLib(type: Copy) { into "${distTarget}/libs/" } -task copyExtensions(type: Copy) { +// Sync, not Copy: the allow-list below is only meaningful if the destination mirrors it exactly. +// A Copy leaves behind archives and exploded directories belonging to extensions that have since +// been withdrawn from the release set, so an incremental build keeps shipping them and the +// allow-list silently stops being enforced. Sync removes anything not produced by this build, +// including the subdirectories explodeExtensions creates, which that task then recreates. +task copyExtensions(type: Sync) { group 'Build' - description 'Copy extension ZIP archives to extensions/ directory' + description 'Mirror the released extension ZIP archives into the extensions/ directory' into "${distTarget}/extensions/" } diff --git a/btrace-gradle-plugin/build.gradle b/btrace-gradle-plugin/build.gradle index 3cf5ef53b..b8ec7bf34 100644 --- a/btrace-gradle-plugin/build.gradle +++ b/btrace-gradle-plugin/build.gradle @@ -26,9 +26,11 @@ repositories { dependencies { implementation gradleApi() implementation localGroovy() - // Match versions commonly cached in this repository to avoid extra fetches - implementation 'org.ow2.asm:asm:9.9.1' - implementation 'org.ow2.asm:asm-tree:9.9.1' + // This is an included build and cannot access the root version catalog, so the ASM version is + // duplicated here. It must stay in sync with version('asm', ...) in the root settings.gradle; + // AsmVersionSyncTest fails the build if the two drift apart. + implementation 'org.ow2.asm:asm:9.10.1' + implementation 'org.ow2.asm:asm-tree:9.10.1' // This is an included build and cannot access the root version catalog. Keep its tests on the // last Java 8-compatible JUnit line; the production plugin remains Java 11-compatible. @@ -47,6 +49,10 @@ test { javaLauncher = javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(17) } + // AsmVersionSyncTest compares this build's hard-coded ASM version against the root version + // catalog; hand it both files rather than let it guess from the working directory. + systemProperty 'btrace.test.pluginBuildFile', file('build.gradle').absolutePath + systemProperty 'btrace.test.rootSettingsFile', rootDir.parentFile.toPath().resolve('settings.gradle').toString() } gradlePlugin { diff --git a/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java b/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java new file mode 100644 index 000000000..6db8b5d6e --- /dev/null +++ b/btrace-gradle-plugin/src/test/java/io/btrace/gradle/AsmVersionSyncTest.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2008, 2026, Jaroslav Bachorik . + * All rights reserved. + * + * Licensed 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 + * + * https://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 io.btrace.gradle; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Guards the one dependency version this build is forced to duplicate. + * + *

{@code btrace-gradle-plugin} is an included build, so it cannot resolve the root build's + * version catalog and has to hard-code its ASM coordinates. That duplication has already drifted + * once: a release-prep commit rebased on a stale checkout silently reverted the plugin's ASM to an + * older version than the catalog, and nothing failed. This test turns that silent drift into a + * build failure. + */ +class AsmVersionSyncTest { + + private static final Pattern CATALOG_ASM_VERSION = + Pattern.compile("version\\s*\\(\\s*'asm'\\s*,\\s*'([^']+)'\\s*\\)"); + private static final Pattern PLUGIN_ASM_COORDINATE = + Pattern.compile("['\"]org\\.ow2\\.asm:(asm[\\w-]*):([^'\"]+)['\"]"); + + @Test + @DisplayName("plugin ASM version matches the root version catalog") + void asmVersionMatchesRootCatalog() throws IOException { + Path rootSettings = requiredPath("btrace.test.rootSettingsFile"); + Assumptions.assumeTrue( + Files.isRegularFile(rootSettings), + "root settings.gradle not reachable - plugin is being built standalone"); + + String catalogVersion = catalogAsmVersion(read(rootSettings)); + Set pluginVersions = pluginAsmVersions(read(pluginBuildFile())); + + assertTrue(!pluginVersions.isEmpty(), "no org.ow2.asm coordinates found in the plugin build"); + assertEquals( + 1, + pluginVersions.size(), + "btrace-gradle-plugin declares conflicting ASM versions: " + pluginVersions); + assertEquals( + catalogVersion, + pluginVersions.iterator().next(), + "btrace-gradle-plugin's hard-coded ASM version has drifted from version('asm', ...) " + + "in the root settings.gradle. Update btrace-gradle-plugin/build.gradle to " + + "match, or update the catalog if the plugin is intentionally pinned."); + } + + @Test + @DisplayName("every ASM artifact in the plugin shares one version") + void allAsmArtifactsShareOneVersion() throws IOException { + String pluginBuild = read(pluginBuildFile()); + Matcher m = PLUGIN_ASM_COORDINATE.matcher(pluginBuild); + Set artifacts = new LinkedHashSet<>(); + Set versions = new LinkedHashSet<>(); + while (m.find()) { + artifacts.add(m.group(1)); + versions.add(m.group(2)); + } + assertTrue(artifacts.contains("asm"), "expected an org.ow2.asm:asm dependency"); + assertTrue(artifacts.contains("asm-tree"), "expected an org.ow2.asm:asm-tree dependency"); + assertEquals(1, versions.size(), "ASM artifacts must share a single version: " + versions); + } + + private static String catalogAsmVersion(String settings) { + Matcher m = CATALOG_ASM_VERSION.matcher(settings); + assertTrue(m.find(), "version('asm', ...) not found in the root settings.gradle"); + return m.group(1); + } + + private static Set pluginAsmVersions(String buildFile) { + Matcher m = PLUGIN_ASM_COORDINATE.matcher(buildFile); + Set versions = new LinkedHashSet<>(); + while (m.find()) { + versions.add(m.group(2)); + } + return versions; + } + + private static Path pluginBuildFile() { + Path buildFile = requiredPath("btrace.test.pluginBuildFile"); + assertTrue(Files.isRegularFile(buildFile), "cannot locate btrace-gradle-plugin/build.gradle"); + return buildFile; + } + + private static Path requiredPath(String property) { + String value = System.getProperty(property); + assertTrue( + value != null && !value.isEmpty(), + "system property " + property + " is not set; it is wired up by the test task"); + return Paths.get(value); + } + + private static String read(Path path) throws IOException { + List lines = Files.readAllLines(path, StandardCharsets.UTF_8); + return String.join("\n", lines); + } +} diff --git a/integration-tests/src/test/java/tests/BTraceFunctionalTests.java b/integration-tests/src/test/java/tests/BTraceFunctionalTests.java index 4c361dd7e..4e296c0a8 100644 --- a/integration-tests/src/test/java/tests/BTraceFunctionalTests.java +++ b/integration-tests/src/test/java/tests/BTraceFunctionalTests.java @@ -21,7 +21,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -import static org.junit.jupiter.api.Assumptions.assumeFalse; import static org.junit.jupiter.api.Assumptions.assumeTrue; import io.btrace.client.Client; @@ -62,6 +61,13 @@ * @author Jaroslav Bachorik */ public class BTraceFunctionalTests extends RuntimeTest { + /** + * The client logs this once a probe is installed and running. Tests whose expected output can + * only appear after the target is torn down gate on it, so the teardown never races an attach + * that is still in progress. + */ + private static final String PROBE_STARTED_MARKER = "Successfully started BTrace probe"; + @BeforeAll public static void setup() throws Exception { classSetup(); @@ -73,6 +79,31 @@ public void reset() { super.reset(); } + /** + * The feature version of the JDK under test. + * + *

Deliberately not routed through {@link RuntimeTest#resolveTestJavaHome()}: that method's + * {@code java.home} fallback would make this read a {@code release} file off disk purely to + * rediscover the version of the JVM already executing this code, which is what {@code + * java.runtime.version} reports directly. Only an explicitly configured test JDK can differ. + * + * @return the version string of the JDK the target application will run under + */ + private static String targetJdkVersion() throws IOException { + String configured = System.getenv("TEST_JAVA_HOME"); + if (configured == null) { + configured = System.getenv("JAVA_TEST_HOME"); + } + String rtVersion = System.getProperty("java.runtime.version", ""); + if (configured == null) { + return rtVersion; + } + Properties releaseProps = new Properties(); + releaseProps.load( + Files.newInputStream(new File(configured + File.separator + "release").toPath())); + return releaseProps.getProperty("JAVA_VERSION", rtVersion).replace("\"", ""); + } + @Test public void testOSMBean() throws Exception { isUnsafe = true; @@ -174,12 +205,20 @@ public void validate(String stdout, String stderr, int retcode, String jfrFile) @Test public void testOnExit() throws Exception { - timeout = 3500; + // @OnExit can only fire once the target JVM exits, and the harness signals that shutdown only + // after the completion gate returns - so waiting for "onexit" here can never be satisfied and + // always burns the full timeout. Worse, when the machine is loaded the client may still be + // attaching when that timeout expires, and the teardown then pulls the target out from under + // it: the client reports "No such process" on stderr and the run fails. Gate on the probe + // being live instead, which is both deterministic and quicker. testDynamic( "resources.Main", "btrace/OnExitTest.java", - Completion.untilContains("onexit"), + Completion.untilContains(PROBE_STARTED_MARKER), (stdout, stderr, retcode, jfrFile) -> { + assertTrue( + stdout.contains(PROBE_STARTED_MARKER), + "probe did not start before the target was torn down; stdout: " + stdout); assertFalse(stdout.contains("FAILED"), "Script should not have failed"); assertTrue(stderr.isEmpty(), "Non-empty stderr"); assertTrue(stdout.contains("onexit")); @@ -272,21 +311,7 @@ public void locateTracePrefersCurrentBuildOutput() { @Test public void testTraceAll() throws Exception { - String testJavaHome = System.getenv().get("TEST_JAVA_HOME"); - if (testJavaHome == null) testJavaHome = System.getenv().get("JAVA_TEST_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getProperty("java.home"); - } - } - - assumeFalse(testJavaHome == null); - - Properties releaseProps = new Properties(); - releaseProps.load( - Files.newInputStream(new File(testJavaHome + File.separator + "release").toPath())); - String rtVersion = releaseProps.getProperty("JAVA_VERSION").replace("\"", ""); + String rtVersion = targetJdkVersion(); if (!isVersionSafeForTraceAll(rtVersion)) { System.err.println("Skipping test for JDK " + rtVersion); return; @@ -444,15 +469,7 @@ public void validate(String stdout, String stderr, int retcode, String jfrFile) @Test public void testJfr() throws Exception { - String rtVersion = System.getProperty("java.runtime.version", ""); - String testJavaHome = System.getenv().get("TEST_JAVA_HOME"); - if (testJavaHome == null) testJavaHome = System.getenv().get("JAVA_TEST_HOME"); - if (testJavaHome != null) { - Properties releaseProps = new Properties(); - releaseProps.load( - Files.newInputStream(new File(testJavaHome + File.separator + "release").toPath())); - rtVersion = releaseProps.getProperty("JAVA_VERSION").replace("\"", ""); - } + String rtVersion = targetJdkVersion(); if (!isVersionSafeForJfr(rtVersion)) { // skip the test for 8.0.* because of missing support // skip all non-LTS versions (except the last one) diff --git a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java index 8a84679dd..86d72ef79 100644 --- a/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java +++ b/integration-tests/src/test/java/tests/Issue884PublishedFatAgentE2ETest.java @@ -28,6 +28,8 @@ import java.util.concurrent.TimeUnit; import java.util.jar.Attributes; import java.util.jar.JarFile; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -37,8 +39,24 @@ class Issue884PublishedFatAgentE2ETest { private static final String VERSION = "3.0.0"; - /** Must match the ASM version the Gradle plugin declares in {@code btrace-gradle-plugin}. */ - private static final String ASM_VERSION = "9.9.1"; + /** + * Matches {@code version('asm', ..)} in the root {@code settings.gradle}, which is also what + * {@code btrace-gradle-plugin} declares. The external consumer build resolves from the staged + * repository alone, so an ASM version staged here that the plugin does not depend on leaves the + * consumer unable to resolve it. Deriving the value keeps that from drifting. + */ + private static final Pattern CATALOG_ASM_VERSION = + Pattern.compile("version\\s*\\(\\s*'asm'\\s*,\\s*'([^']+)'\\s*\\)"); + + /** + * Budget for the nested Gradle builds this test forks. + * + *

Each fork resolves plugins and dependencies and may compile from a cold cache. 90 seconds + * was enough on a warm, idle machine but expired routinely on a loaded one, surfacing as + * "publication did not finish" rather than as anything diagnostic. This bounds a hung build + * without racing a merely slow one. + */ + private static final long NESTED_BUILD_TIMEOUT_SECONDS = 300; @TempDir Path temporaryDirectory; @@ -106,8 +124,25 @@ private void publishFixtureRepository(Path root, Path repository) throws Excepti publishPluginArtifacts(root, repository); publishArtifact(repository, "io/btrace", "btrace", VERSION, engineJar); - publishDependency(repository, "org.ow2.asm", "asm", ASM_VERSION); - publishDependency(repository, "org.ow2.asm", "asm-tree", ASM_VERSION); + String asmVersion = catalogAsmVersion(root); + publishDependency(repository, "org.ow2.asm", "asm", asmVersion); + publishDependency(repository, "org.ow2.asm", "asm-tree", asmVersion); + } + + /** + * Reads the ASM version from the root version catalog rather than restating it here. + * + * @param root the repository root + * @return the ASM version the Gradle plugin resolves against + */ + private String catalogAsmVersion(Path root) throws IOException { + Path settings = root.resolve("settings.gradle"); + assertTrue(Files.isRegularFile(settings), "missing root settings.gradle: " + settings); + Matcher matcher = + CATALOG_ASM_VERSION.matcher( + new String(Files.readAllBytes(settings), StandardCharsets.UTF_8)); + assertTrue(matcher.find(), "version('asm', ..) not found in " + settings); + return matcher.group(1); } private void publishPluginArtifacts(Path root, Path repository) throws Exception { @@ -122,7 +157,9 @@ private void publishPluginArtifacts(Path root, Path repository) throws Exception .directory(root.toFile()) .redirectErrorStream(true) .start(); - assertTrue(process.waitFor(90, TimeUnit.SECONDS), "plugin publication did not finish"); + assertTrue( + process.waitFor(NESTED_BUILD_TIMEOUT_SECONDS, TimeUnit.SECONDS), + "plugin publication did not finish"); String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); assertTrue(process.exitValue() == 0, output); } @@ -226,7 +263,9 @@ private Path buildExternalConsumer(Path root, Path repository) throws Exception .directory(root.toFile()) .redirectErrorStream(true) .start(); - assertTrue(process.waitFor(90, TimeUnit.SECONDS), "external consumer did not finish"); + assertTrue( + process.waitFor(NESTED_BUILD_TIMEOUT_SECONDS, TimeUnit.SECONDS), + "external consumer did not finish"); String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); assertTrue(process.exitValue() == 0, output); assertTrue(output.contains("Staged masked BTrace engine"), output); diff --git a/integration-tests/src/test/java/tests/RuntimeTest.java b/integration-tests/src/test/java/tests/RuntimeTest.java index afad9a18f..31fe2a1f7 100644 --- a/integration-tests/src/test/java/tests/RuntimeTest.java +++ b/integration-tests/src/test/java/tests/RuntimeTest.java @@ -115,6 +115,33 @@ public abstract class RuntimeTest { private ProtocolSettings clientProtocolSettings = FORCED_V2_PROTOCOL; private ProtocolSettings agentProtocolSettings = FORCED_V2_PROTOCOL; + /** + * Resolves the JDK that the target application and the BTrace client run under. + * + *

{@code TEST_JAVA_HOME} wins, then its {@code JAVA_TEST_HOME} alias, then {@code JAVA_HOME}, + * and finally the JVM running the tests. Every call site used to inline this chain with a + * different tail - most omitted {@code java.home}, one also omitted the alias - so a bare + * environment failed in some code paths while succeeding in others. + * + * @return the resolved JDK home + */ + protected static String resolveTestJavaHome() { + String[] candidates = { + System.getenv("TEST_JAVA_HOME"), + System.getenv("JAVA_TEST_HOME"), + System.getenv("JAVA_HOME"), + System.getProperty("java.home") + }; + for (String candidate : candidates) { + if (candidate != null && !candidate.trim().isEmpty()) { + return candidate; + } + } + throw new IllegalStateException( + "Cannot resolve a JDK for the integration tests: none of TEST_JAVA_HOME, JAVA_TEST_HOME," + + " JAVA_HOME or the java.home system property is set"); + } + public static void classSetup() { if (System.getProperty("btrace.comm.protocol") == null) { System.setProperty("btrace.comm.protocol", "2"); @@ -161,22 +188,7 @@ public static void classSetup() { Assertions.assertNotNull(clientClassPath); String toolsjar = null; - // Accept both TEST_JAVA_HOME (preferred) and JAVA_TEST_HOME as aliases - // TEST_JAVA_HOME has the highest precedence - javaHome = System.getenv("TEST_JAVA_HOME"); - if (javaHome == null) { - javaHome = System.getenv("JAVA_TEST_HOME"); - } - if (javaHome == null) { - javaHome = System.getenv("JAVA_HOME"); - } - if (javaHome == null) { - javaHome = System.getProperty("java.home"); - } - if (javaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } - javaHome = javaHome.replace("/jre", ""); + javaHome = resolveTestJavaHome().replace("/jre", ""); Path toolsJarPath = Paths.get(javaHome, "lib", "tools.jar"); if (Files.exists(toolsJarPath)) { @@ -304,14 +316,7 @@ public void testDynamicOneliner( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); System.out.println("===> test java: " + testJavaHome); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", cp)); @@ -500,14 +505,7 @@ public void testDynamic( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); System.out.println("===> test java: " + testJavaHome); String jfrFile = null; List args = @@ -702,14 +700,7 @@ public void testStartup( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - if (testJavaHome == null) { - testJavaHome = System.getenv("JAVA_TEST_HOME"); - } - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", targetAppCp)); @@ -1046,11 +1037,7 @@ private TestApp launchTestAppInternal( debugBTrace = true; debugTestApp = true; } - String testJavaHome = System.getenv("TEST_JAVA_HOME"); - testJavaHome = testJavaHome != null ? testJavaHome : System.getenv("JAVA_HOME"); - if (testJavaHome == null) { - throw new IllegalStateException("Missing TEST_JAVA_HOME or JAVA_HOME env variables"); - } + String testJavaHome = resolveTestJavaHome(); String jfrFile = null; List args = new ArrayList<>(Arrays.asList(testJavaHome + "/bin/java", "-cp", cp)); if (attachDebugger) {