Copilot commented on code in PR #15698: URL: https://github.com/apache/grails-core/pull/15698#discussion_r3322150862
########## grails-shell-cli/src/main/groovy/org/grails/cli/gradle/RunningApplicationRegistry.groovy: ########## @@ -0,0 +1,127 @@ +/* + * 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 + * + * 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 org.grails.cli.gradle + +import java.util.concurrent.ConcurrentHashMap + +import groovy.transform.CompileStatic + +import org.gradle.tooling.CancellationTokenSource + +/** + * Tracks the {@link CancellationTokenSource} of Grails applications started by the + * CLI via the {@code run-app} command (an asynchronous Gradle {@code bootRun} build). + * + * <p>This allows the {@code stop-app} command to cancel the underlying Gradle build + * without exiting the interactive CLI, providing a CLI only shutdown mechanism that + * does not rely on the Spring Boot Actuator shutdown endpoint or JMX.</p> + * + * <p>A {@code run-app} build registers its token before the build runs and removes it + * when the build finishes (whether it completes normally, fails, or is cancelled). + * {@link #stopAll()} only requests cancellation; it never removes tokens directly so + * that the build remains responsible for its own lifecycle.</p> + * + * @author Apache Grails Team + * @since 7.0.0 + */ +@CompileStatic +class RunningApplicationRegistry { + + private static final Set<CancellationTokenSource> RUNNING = ConcurrentHashMap.newKeySet() + + private RunningApplicationRegistry() { + } + + /** + * Registers the cancellation token source of a running application. + * + * @param tokenSource the token source backing the running build + */ + static void register(CancellationTokenSource tokenSource) { + if (tokenSource != null) { + RUNNING.add(tokenSource) + } + } + + /** + * Removes a previously registered cancellation token source. This should be called + * by the build once it has finished, regardless of how it terminated. + * + * @param tokenSource the token source to remove + */ + static void deregister(CancellationTokenSource tokenSource) { + if (tokenSource != null) { + RUNNING.remove(tokenSource) + } + } + + /** + * @return {@code true} if at least one application started via {@code run-app} is running + */ + static boolean isApplicationRunning() { + !RUNNING.isEmpty() + } + + /** + * Requests cancellation of every running application. The registered token sources are + * not removed here; each running build removes its own token when it terminates. + * + * @return {@code true} if at least one running application was found and cancellation requested + */ + static boolean stopAll() { + if (RUNNING.isEmpty()) { + return false + } + // Snapshot to avoid surprises if a build deregisters concurrently while we iterate + List<CancellationTokenSource> tokenSources = new ArrayList<>(RUNNING) + for (CancellationTokenSource tokenSource : tokenSources) { + try { + tokenSource.cancel() + } + catch (Throwable ignored) { + // best effort: continue cancelling the remaining applications + } Review Comment: `catch (Throwable ignored)` silently swallows any failure to cancel a build. If `tokenSource.cancel()` ever fails (e.g. due to a Tooling API issue), the user will see "Application stopped." after the await times out without any hint of what went wrong. Consider logging via `GrailsConsole.getInstance().verbose(...)` or `error(...)` so the failure isn't completely invisible during diagnosis. ########## grails-shell-cli/src/main/groovy/org/grails/cli/gradle/RunningApplicationRegistry.groovy: ########## @@ -0,0 +1,127 @@ +/* + * 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 + * + * 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 org.grails.cli.gradle + +import java.util.concurrent.ConcurrentHashMap + +import groovy.transform.CompileStatic + +import org.gradle.tooling.CancellationTokenSource + +/** + * Tracks the {@link CancellationTokenSource} of Grails applications started by the + * CLI via the {@code run-app} command (an asynchronous Gradle {@code bootRun} build). + * + * <p>This allows the {@code stop-app} command to cancel the underlying Gradle build + * without exiting the interactive CLI, providing a CLI only shutdown mechanism that + * does not rely on the Spring Boot Actuator shutdown endpoint or JMX.</p> + * + * <p>A {@code run-app} build registers its token before the build runs and removes it + * when the build finishes (whether it completes normally, fails, or is cancelled). + * {@link #stopAll()} only requests cancellation; it never removes tokens directly so + * that the build remains responsible for its own lifecycle.</p> + * + * @author Apache Grails Team + * @since 7.0.0 + */ +@CompileStatic +class RunningApplicationRegistry { + + private static final Set<CancellationTokenSource> RUNNING = ConcurrentHashMap.newKeySet() + + private RunningApplicationRegistry() { + } + + /** + * Registers the cancellation token source of a running application. + * + * @param tokenSource the token source backing the running build + */ + static void register(CancellationTokenSource tokenSource) { + if (tokenSource != null) { + RUNNING.add(tokenSource) + } + } + + /** + * Removes a previously registered cancellation token source. This should be called + * by the build once it has finished, regardless of how it terminated. + * + * @param tokenSource the token source to remove + */ + static void deregister(CancellationTokenSource tokenSource) { + if (tokenSource != null) { + RUNNING.remove(tokenSource) + } + } + + /** + * @return {@code true} if at least one application started via {@code run-app} is running + */ + static boolean isApplicationRunning() { + !RUNNING.isEmpty() + } + + /** + * Requests cancellation of every running application. The registered token sources are + * not removed here; each running build removes its own token when it terminates. + * + * @return {@code true} if at least one running application was found and cancellation requested + */ + static boolean stopAll() { + if (RUNNING.isEmpty()) { + return false + } + // Snapshot to avoid surprises if a build deregisters concurrently while we iterate + List<CancellationTokenSource> tokenSources = new ArrayList<>(RUNNING) + for (CancellationTokenSource tokenSource : tokenSources) { + try { + tokenSource.cancel() + } + catch (Throwable ignored) { + // best effort: continue cancelling the remaining applications + } + } + return true + } + + /** + * Waits up to the given timeout for all running applications to terminate, i.e. for the + * cancelled builds to finish tearing down and deregister their token sources. + * + * @param timeoutMillis the maximum time to wait in milliseconds + * @return {@code true} if all applications stopped within the timeout, {@code false} otherwise + */ + static boolean awaitStop(long timeoutMillis) { + long deadline = System.currentTimeMillis() + timeoutMillis + while (!RUNNING.isEmpty()) { + if (System.currentTimeMillis() >= deadline) { + return false + } + try { + Thread.sleep(100) + } + catch (InterruptedException e) { + Thread.currentThread().interrupt() + return RUNNING.isEmpty() + } + } + return true + } Review Comment: `awaitStop` uses a 100 ms busy-wait poll loop. Since `RUNNING` is only mutated by `register`/`deregister`, a small `synchronized` monitor with `wait`/`notifyAll` (or a `CountDownLatch`/`Phaser`) would let `stop-app` react immediately when the cancelled build finishes, rather than introducing up to 100 ms of additional latency and unnecessary wake-ups. Optional, but it would also make the timeout handling more deterministic. -- 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]
