Copilot commented on code in PR #3372: URL: https://github.com/apache/maven-surefire/pull/3372#discussion_r3358918055
########## surefire-extensions-api/src/main/java/org/apache/maven/surefire/extensions/ForkedProcessTimeoutExtension.java: ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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 org.apache.maven.surefire.extensions; + +import org.apache.maven.surefire.api.suite.RunResult; + +/** + * Extension point invoked when a forked Surefire test JVM exceeds its + * configured {@code forkedProcessTimeoutInSeconds}. + * <p> + * Implementations are discovered via the standard {@link java.util.ServiceLoader} + * mechanism from the <em>plugin classpath</em>. To register an extension, add a + * file + * {@code META-INF/services/org.apache.maven.surefire.extensions.ForkedProcessTimeoutExtension} + * to your extension JAR and declare it as a {@code <dependency>} of + * {@code maven-surefire-plugin} (or {@code maven-failsafe-plugin}) in the + * project POM. + * <p> + * The primary motivation is to capture diagnostic information (for example, a + * {@code jstack} thread dump) about a test JVM that is about to be killed so + * the root cause of a deadlock or hang can be investigated. + * <p> + * <strong>Lifecycle:</strong> + * <ol> + * <li>{@link #onTimeoutDetected(ForkedProcessTimeoutContext)} is invoked + * synchronously immediately after Surefire detects the timeout but + * <em>before</em> the {@code KILL} shutdown command is sent. The forked JVM + * is therefore still alive at this point, which makes utilities such as + * {@code jstack} usable.</li> + * <li>{@link #onForkExited(ForkedProcessTimeoutContext, RunResult)} is + * invoked from the plugin after the forked JVM has actually exited.</li> + * </ol> + * Implementations must not throw checked exceptions; any {@link Throwable} + * raised by a callback is caught by Surefire, logged at warn level, and does + * not affect the test result. + * <p> + * Callbacks run on a Surefire-internal scheduler thread. Implementations + * should complete quickly and must not block indefinitely; Surefire applies + * an internal time limit to each callback invocation. Review Comment: The Javadoc contradicts the method signatures and current implementation: the SPI methods declare `throws Exception`, and callbacks are dispatched via an internal executor (not directly on the scheduler thread). The documentation should reflect that exceptions are allowed (and will be caught/logged) and that callbacks are executed on Surefire-managed threads with a time limit. ########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/extensions/timeout/JstackTimeoutExtension.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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 org.apache.maven.plugin.surefire.extensions.timeout; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.util.concurrent.TimeUnit; + +import org.apache.maven.plugin.surefire.log.api.ConsoleLogger; +import org.apache.maven.surefire.api.suite.RunResult; +import org.apache.maven.surefire.extensions.ForkedProcessTimeoutContext; +import org.apache.maven.surefire.extensions.ForkedProcessTimeoutExtension; +import org.apache.maven.surefire.shared.lang3.SystemUtils; + +/** + * Built-in {@link ForkedProcessTimeoutExtension} that captures a {@code jstack} + * thread dump of the forked test JVM just before it is killed because of + * {@code forkedProcessTimeoutInSeconds}. + * <p> + * The output is written to + * {@code <reportsDirectory>/surefire-timeout-jstack-<forkNumber>-<pid>.txt}. + * <p> + * <strong>Activation:</strong> this extension is registered via + * {@code META-INF/services} but is <em>disabled by default</em>. To enable it + * set the system property {@code surefire.timeout.jstack.enabled=true} on the + * Maven process (for example with {@code MAVEN_OPTS} or + * {@code -Dsurefire.timeout.jstack.enabled=true}). The {@code jstack} binary + * is resolved from {@code JAVA_HOME/bin/jstack} when {@code JAVA_HOME} is set, + * otherwise the {@code jstack} on {@code PATH} is used. + * + * @since 3.6.0 + */ +public class JstackTimeoutExtension implements ForkedProcessTimeoutExtension { + + /** System property that enables this extension. */ + public static final String ENABLED_PROPERTY = "surefire.timeout.jstack.enabled"; + + /** + * Extension-context key that enables this extension from the POM via the + * {@code forkedProcessTimeoutExtensionContext} Mojo parameter. Set to + * {@code true} to enable. When either this key or {@link #ENABLED_PROPERTY} + * is set to {@code true}, the extension runs. + */ + public static final String ENABLED_KEY = "jstack.enabled"; + + /** + * Extension context key that overrides the directory where the + * {@code surefire-timeout-jstack-*.txt} files are written. When the key is + * missing or blank, the Surefire reports directory is used. + */ + public static final String OUTPUT_LOCATION_KEY = "jstack.output.location"; + + /** Wall-clock timeout for the jstack subprocess. */ + static final int JSTACK_TIMEOUT_SECONDS = 20; + + @Override + public void onTimeoutDetected(ForkedProcessTimeoutContext context) { + ConsoleLogger logger = context.getConsoleLogger(); + if (!isEnabled(context)) { + logger.debug("JstackTimeoutExtension disabled (set -D" + ENABLED_PROPERTY + "=true or POM key " + + ENABLED_KEY + "=true to enable)"); + return; + } + long pid = context.getPid(); + if (pid <= 0L) { + logger.warning("JstackTimeoutExtension: PID of forked JVM unknown (Java 8 or unsupported platform); " + + "skipping jstack for fork " + context.getForkNumber()); + return; + } + File jstack = resolveJstackBinary(); + if (jstack == null) { + logger.warning("JstackTimeoutExtension: cannot find jstack in JAVA_HOME or PATH; " + + "skipping jstack for fork " + context.getForkNumber() + " (pid=" + pid + ")"); Review Comment: This warning message is slightly misleading: `resolveJstackBinary()` also checks `java.home` (and its parent JDK bin) in addition to `JAVA_HOME`/`PATH`. Tweaking the message avoids confusing users who rely on the default `java.home` layout. ########## surefire-its/src/test/resources/surefire-838-timeout-extension/src/test/java/surefire838/SleepingTest.java: ########## @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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 surefire838; + +import org.junit.Test; + +public class SleepingTest { + + @Test + public void sleepsLongerThanTimeout() throws Exception { + Thread.sleep(120_000L); Review Comment: This test sleeps for 2 minutes. If the timeout/kill path regresses, this IT can stall the build for a long time (and may hit CI job timeouts). A shorter sleep still exceeds the 5s `forkedProcessTimeoutInSeconds` while keeping the worst-case runtime bounded. ########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/extensions/timeout/JstackTimeoutExtension.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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 org.apache.maven.plugin.surefire.extensions.timeout; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.util.concurrent.TimeUnit; + +import org.apache.maven.plugin.surefire.log.api.ConsoleLogger; +import org.apache.maven.surefire.api.suite.RunResult; +import org.apache.maven.surefire.extensions.ForkedProcessTimeoutContext; +import org.apache.maven.surefire.extensions.ForkedProcessTimeoutExtension; +import org.apache.maven.surefire.shared.lang3.SystemUtils; + +/** + * Built-in {@link ForkedProcessTimeoutExtension} that captures a {@code jstack} + * thread dump of the forked test JVM just before it is killed because of + * {@code forkedProcessTimeoutInSeconds}. + * <p> + * The output is written to + * {@code <reportsDirectory>/surefire-timeout-jstack-<forkNumber>-<pid>.txt}. + * <p> + * <strong>Activation:</strong> this extension is registered via + * {@code META-INF/services} but is <em>disabled by default</em>. To enable it + * set the system property {@code surefire.timeout.jstack.enabled=true} on the + * Maven process (for example with {@code MAVEN_OPTS} or + * {@code -Dsurefire.timeout.jstack.enabled=true}). The {@code jstack} binary + * is resolved from {@code JAVA_HOME/bin/jstack} when {@code JAVA_HOME} is set, + * otherwise the {@code jstack} on {@code PATH} is used. Review Comment: The class-level Javadoc states that `jstack` is resolved only from `JAVA_HOME/bin` or `PATH`, but the implementation also searches `java.home/bin` and its parent JDK bin (Java 8 layout). Please update the documentation to match the actual lookup order so users know where Surefire searches. -- 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]
