codeconsole commented on code in PR #16536: URL: https://github.com/apache/grails-core/pull/16536#discussion_r4198426221
########## grails-startup-progress/src/main/java/org/apache/grails/startup/StartupProgressServer.java: ########## @@ -0,0 +1,149 @@ +/* + * 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.apache.grails.startup; + +import java.io.IOException; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpServer; + +/** + * Holds the application's port until the embedded web server is about to take it over, answering every + * request on it with the {@link StartupProgressResponder}: the progress page for a browser, the status + * the page polls for, and a plain {@code 503} for anything else. + */ +final class StartupProgressServer { + + private final InetSocketAddress address; + + private final StartupProgressResponder responder; + + private HttpServer server; + + private ExecutorService executor; + + private volatile boolean running; + + StartupProgressServer(InetAddress address, int port, StartupProgressResponder responder) { + this.address = new InetSocketAddress(address, port); + this.responder = responder; + } + + /** + * Binds the port and starts answering. A stopped server can be started again. + * + * @throws IOException when the port cannot be bound, typically because something else holds it + */ + void start() throws IOException { + if (running) { + return; + } + HttpServer httpServer = HttpServer.create(address, 0); + ExecutorService pool = Executors.newFixedThreadPool(2, runnable -> { + Thread thread = new Thread(runnable, "grails-startup-progress"); + thread.setDaemon(true); + return thread; + }); + httpServer.createContext("/", this::handle); + httpServer.setExecutor(pool); + httpServer.start(); Review Comment: `HttpServer.start()` creates its `HTTP-Dispatcher` thread without setting the daemon flag, so the thread inherits it from the caller. Here the caller is the main thread, so the dispatcher is non-daemon; the daemon thread factory only covers the handler pool. On JDK 21, a started `HttpServer` keeps the JVM running after `main` returns. That matters for any run that binds the port but never reaches the hand-off lifecycle, `started()` or `failed()`. `SpringApplication.handleRunFailure` returns early on an `AbandonedRunException` without calling the run listeners' `failed`, and Spring Boot's AOT processor throws exactly that from `contextLoaded`. So with `grails.startup.progress.enabled: true` set at the top level of `application.yml` (the guide describes turning it on in production), `processAot` binds the port and never exits. Could the server be started from a daemon thread, so the dispatcher can never keep the JVM running? ########## grails-startup-progress/src/main/java/org/apache/grails/startup/StartupProgressResponder.java: ########## @@ -0,0 +1,212 @@ +/* + * 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.apache.grails.startup; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.List; + +/** + * Decides how the startup progress answers a request, and answers it, for both of the servers that answer + * for it: the one that holds the application's port until the web server starts, and the filter inside the + * application once the web server has the port. Each hands its requests over as an {@link Exchange}. + * + * <p>The two differ only in which requests they take. The server that holds the port before the web server + * starts takes every request, since nothing else could answer one. The filter takes the startup progress's + * own paths and, while the application starts and the progress page is served, browser page loads; every + * other request, such as an API call or a request the application makes to itself from {@code BootStrap}, + * reaches the application as it always has.</p> + * + * <p>A request taken is answered the same way by either:</p> + * <ul> + * <li>at the status path, with the status the progress page polls for, or once the application is ready, + * with only that it is ready</li> + * <li>at the report path, with the report as data to a client asking for JSON, and once the application is + * ready, with the report page</li> + * <li>when it carries the token from the log, by signing the browser in and sending it on to the address + * without the token</li> + * <li>otherwise, while the application starts, with the progress page for a browser and a plain + * {@code 503} for anything else</li> + * </ul> + */ +final class StartupProgressResponder { + + /** Which requests a server takes, beyond the startup progress's own paths. */ + enum Takes { + + /** Every request, as the server that holds the port before the web server starts does. */ + EVERY_REQUEST, + + /** Browser page loads while the application starts, as the filter does when the progress page is served. */ + PAGE_LOADS, + + /** None, as the filter does when only the report is served. */ + OWN_PATHS_ONLY + } + + /** A request and its response, as a server that answers for the startup progress hands them over. */ + interface Exchange { + + String method(); + + /** The path asked for, undecoded, with the context path. */ + String path(); + + /** The query, undecoded, or {@code null} when there is none. */ + String query(); + + String header(String name); + + List<String> headers(String name); + + void setHeader(String name, String value); + + /** + * Sends the status, headers and body, all of it before returning. + * + * @param contentType the type of the body, or {@code null} when there is no body + */ + void send(int status, String contentType, byte[] body) throws IOException; + } + + private static final byte[] NO_BODY = new byte[0]; + + private final StartupProgress progress; + + private final StartupProgressPage page; + + private final StartupAccess access; + + private final String statusPath; + + private final String reportPath; + + /** + * @param statusPath the full path the progress page polls + * @param reportPath the full path of the startup report, or {@code null} when it is not served + */ + StartupProgressResponder(StartupProgress progress, StartupProgressPage page, StartupAccess access, String statusPath, String reportPath) { + this.progress = progress; + this.page = page; + this.access = access; + this.statusPath = statusPath; + this.reportPath = reportPath; + } + + /** + * Answers the request if the server takes it, and says whether it did. A request left unanswered is the + * application's. + */ + boolean respond(Exchange exchange, Takes takes) throws IOException { + String path = exchange.path(); + boolean starting = progress.isReporting(); + boolean status = path.equals(statusPath); + boolean report = path.equals(reportPath); + if (status && !starting) { + // a page still polling once the application is ready is told so, rather than the poll reaching an + // application that has no such path and logs every request it cannot map + sendReady(exchange); + return true; + } + boolean everyRequest = takes == Takes.EVERY_REQUEST; + if ((everyRequest || starting || report) && signIn(exchange)) { + return true; + } + boolean pageLoad = isPageLoad(exchange); + if (!everyRequest && !report && !(starting && (status || takes == Takes.PAGE_LOADS && pageLoad))) { + return false; + } + boolean showDetails = access.showsDetails(exchange.headers("Cookie")); + boolean signInForDetails = access.isSignInRequired() && !showDetails; + if (status) { + StartupProgress.Status current = progress.report(showDetails, signInForDetails); + exchange.setHeader(StartupProgress.PHASE_HEADER, current.phase().name()); + send(exchange, 200, "application/json", current.json()); + progress.delivered(current); + } + else if (report && StartupProgressPage.wantsJson(exchange.header("Accept"), exchange.query())) { + send(exchange, 200, "application/json", progress.snapshot(showDetails, signInForDetails).json()); + } + else if (report && !starting) { + setPageHeaders(exchange); + send(exchange, 200, "text/html", page.renderReport(progress.snapshot(showDetails, signInForDetails).json(), showDetails, signInForDetails)); + } + else { + sendStarting(exchange, pageLoad, showDetails, signInForDetails); + } + return true; + } + + /** + * Whether the request is a browser loading a page, which browsers say with the fetch metadata headers + * they send on every navigation. A client that does not send them, such as an HTTP client in application + * code, is never mistaken for one. + */ + private static boolean isPageLoad(Exchange exchange) { Review Comment: Browsers only send `Sec-Fetch-*` headers to potentially trustworthy URLs; the Fetch Metadata algorithm returns before setting them otherwise. That means only `localhost`, `127.0.0.1` and HTTPS get them. A developer may open the app over plain HTTP by machine name or LAN address, for example a VM, WSL, a phone, or `http://devbox:8080`. That navigation carries no fetch metadata. Once the filter has the port, this returns `false` and the page load reaches the application mid-`BootStrap`. The guide says the opposite: "A browser that opens the application while `BootStrap` runs is shown the progress page too". The JDK server phase doesn't have this gap, because `sendStarting` falls back to `Accept: text/html`. When `Sec-Fetch-Mode` is absent, could this fall back to a header only browsers send on navigations, such as `Upgrade-Insecure-Requests: 1`? HTTP clients called from `BootStrap` would still be let through. If not, the guide should say the page is only shown during `BootStrap` on `localhost` and HTTPS. ########## grails-startup-progress/src/main/java/org/apache/grails/startup/StartupProgressRunListener.java: ########## @@ -0,0 +1,339 @@ +/* + * 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.apache.grails.startup; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.URI; +import java.net.URISyntaxException; +import java.time.Duration; +import java.util.List; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.SpringApplicationRunListener; +import org.springframework.boot.WebApplicationType; +import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.boot.web.server.context.WebServerApplicationContext; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.util.StringUtils; + +import grails.util.Environment; + +/** + * Serves a progress page on the application's port from the moment the application context is + * prepared until the application is ready, so a browser pointed at a starting application shows how + * far the start has got instead of failing to connect, and why it failed if it does. + * + * <p>The page polls a status path. Until the web server starts, a small server on the JDK's built-in + * HTTP server answers it; that server is stopped in the lifecycle phase just before the web server's, + * and from then on a filter inside the application answers the same path, and serves the page to + * browsers loading one, through the plugin startup hooks and {@code BootStrap} classes Grails runs after + * the web server starts. Once the application is ready the filter answers the poll with that, and the + * page reloads.</p> + * + * <p>The page is only served for a servlet application starting its own embedded web server on a fixed + * port without SSL, and only when {@code grails.startup.progress.enabled} allows it, which by default it + * does in development mode. If the port cannot be bound the application starts exactly as it would + * without the page.</p> + * + * <p>When {@code grails.startup.progress.openBrowser} is set, a browser is opened on the page as soon as it + * is served, or on the application once it is ready when the page is not served.</p> + * + * <p>When {@code grails.startup.progress.endpoint.enabled} allows it, which by default it does in + * development mode, the application also serves a report of its start for as long as it runs, at + * {@code grails.startup.progress.endpoint.path}. The report does not need the progress page, so it is + * served wherever the application starts its own embedded web server.</p> + * + * @since 8.1 + */ +public class StartupProgressRunListener implements SpringApplicationRunListener { + + private static final Logger LOG = LoggerFactory.getLogger(StartupProgressRunListener.class); + + private static final String PROPERTIES_PREFIX = "grails.startup.progress"; + + private static final int DEFAULT_PORT = 8080; + + /** How long a failed start waits for a watching page to be told why, before it carries on failing. */ + private static final long FAILURE_DELIVERY_TIMEOUT_MILLIS = 2000; + + private final SpringApplication application; + + /** When the run of the application began, which the times in the report are measured from. */ + private final long runStartNanos = System.nanoTime(); + + private StartupProgress progress; + + private StartupAccess access; + + /** The full path of the startup report, while it is served. */ + private String reportPath; + + private StartupProgressServer server; + + private InetAddress address; + + private String contextPath = ""; + + private boolean ssl; + + /** The command that opens a browser, while one is still to be opened for this start. */ + private List<String> browserCommand; + + public StartupProgressRunListener(SpringApplication application, String[] args) { + this.application = application; + } + + @Override + public void contextPrepared(ConfigurableApplicationContext context) { + if (application.getWebApplicationType() != WebApplicationType.SERVLET || + !(context instanceof WebServerApplicationContext) || + !StartupProgressFilter.startsEmbeddedServer(context)) { + return; + } + Binder binder = Binder.get(context.getEnvironment()); + StartupProgressProperties properties; + try { + properties = binder.bindOrCreate(PROPERTIES_PREFIX, StartupProgressProperties.class); + } + catch (RuntimeException ex) { + // nothing else reads these settings, so a mistake in them is said here or not at all + LOG.warn("Not serving startup progress: the {} settings could not be read: {}", PROPERTIES_PREFIX, ex.getMessage()); + return; + } + int port; + try { + port = binder.bind("server.port", Integer.class).orElse(DEFAULT_PORT); + address = binder.bind("server.address", InetAddress.class).orElse(null); + contextPath = contextPath(binder.bind("server.servlet.context-path", String.class).orElse("")); + ssl = isSslEnabled(binder); + } + catch (RuntimeException ex) { + // the web server reports a bad server.* setting far more usefully than this page could + LOG.debug("Not serving startup progress: the server settings could not be read", ex); + return; + } + if (properties.isOpenBrowser()) { + browserCommand = properties.getBrowserCommand(); + } + boolean pageEnabled = properties.isEnabledOrDefault(); + boolean reportEnabled = properties.getEndpoint().isEnabledOrDefault(); + if (!pageEnabled && !reportEnabled) { + return; + } + + String applicationName = applicationName(binder); + StartupProgress startupProgress = new StartupProgress(applicationName, Environment.getGrailsVersion(), runStartNanos); + StartupAccess startupAccess = new StartupAccess(properties.isShowDetailsOrDefault(), port); + String statusPath = path(properties.getStatusPath(), StartupProgressProperties.DEFAULT_STATUS_PATH, "startup progress status"); + StartupProgressPage page = new StartupProgressPage(applicationName, statusPath, Environment.getGrailsVersion()); + reportPath = reportEnabled ? path(properties.getEndpoint().getPath(), StartupProgressProperties.Endpoint.DEFAULT_PATH, "startup report") : null; + if (statusPath.equals(reportPath)) { + // a page polling there would take the report for the application answering, and reload without end + LOG.warn("Not serving the startup report at {}: it is the path the startup progress page polls", reportPath); + reportPath = null; + reportEnabled = false; + } + StartupProgressResponder responder = new StartupProgressResponder(startupProgress, page, startupAccess, statusPath, reportPath); + boolean servingPage = pageEnabled && servePage(responder, port); + if (!servingPage && !reportEnabled) { + reportPath = null; + return; + } + progress = startupProgress; + access = startupAccess; + + context.setApplicationStartup(StartupProgressApplicationStartup.create(context.getApplicationStartup(), startupProgress, context.getBeanFactory())); + ConfigurableListableBeanFactory beanFactory = context.getBeanFactory(); + // the lifecycle beans live as long as the context, so they hold what they act on rather than this listener + StartupProgressServer pageServer = server; + WebServerApplicationContext webContext = (WebServerApplicationContext) context; + beanFactory.registerSingleton("grailsStartupProgressHandOff", StartupProgressLifecycle.beforeWebServer(() -> { + startupProgress.startingWebServer(); + if (pageServer != null) { + pageServer.stop(); + } + })); + beanFactory.registerSingleton("grailsStartupProgressInitializing", StartupProgressLifecycle.afterWebServer(() -> { + startupProgress.initializing(); + if (webContext.getWebServer() != null) { + // a random port is only known now, and names the cookie that signs a browser in to the report + startupAccess.usePort(webContext.getWebServer().getPort()); + } + })); + beanFactory.registerSingleton("grailsStartupProgressFilter", + StartupProgressFilter.registration(responder, servingPage)); + if (servingPage) { + LOG.info("Startup progress is shown at {} until the application is ready", uri("http", port, "/", signInQuery())); + openBrowser("http", port, signInQuery()); + } + } + + /** + * Serves the progress page until the web server takes the port over, and says whether it is served. + */ + private boolean servePage(StartupProgressResponder responder, int port) { + if (port <= 0) { + LOG.debug("Not serving startup progress: server.port {} is not a fixed port", port); + return false; + } + if (ssl) { + LOG.debug("Not serving startup progress: the web server uses SSL"); + return false; + } + StartupProgressServer progressServer = new StartupProgressServer(address, port, responder); Review Comment: A related case: CRaC with `-Dspring.context.checkpoint=onRefresh`. Spring takes that checkpoint in `DefaultLifecycleProcessor.onRefresh()`, before it starts any lifecycle bean. `grailsStartupProgressHandOff` hasn't run yet at that point, so the progress server's listening socket is still open and the checkpoint fails with `CheckpointOpenSocketException`. It's the same open-socket problem we had with MongoDB in #16495. It only happens with the page enabled outside development mode. Still, skipping the page when `SpringProperties.getProperty("spring.context.checkpoint")` is `onRefresh` costs one line. ########## grails-data-hibernate5/dbmigration/src/test/groovy/org/grails/plugins/databasemigration/liquibase/GrailsLiquibaseStartupTaskSpec.groovy: ########## @@ -0,0 +1,199 @@ +/* + * 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.plugins.databasemigration.liquibase + +import java.sql.Connection +import javax.sql.DataSource + +import liquibase.Liquibase +import liquibase.changelog.ChangeSet +import liquibase.changelog.DatabaseChangeLog +import liquibase.changelog.visitor.AbstractChangeExecListener +import liquibase.database.Database +import org.h2.jdbcx.JdbcDataSource +import spock.lang.Specification + +import org.springframework.boot.context.metrics.buffering.BufferingApplicationStartup +import org.springframework.context.support.GenericApplicationContext +import org.springframework.core.metrics.StartupStep + +import grails.boot.StartupTask + +/** + * A database update reports its progress as a {@link StartupTask} when the application's start is recorded, + * as it is when the startup progress page is shown, and runs exactly as before when it is not. + */ +class GrailsLiquibaseStartupTaskSpec extends Specification { + + BufferingApplicationStartup recorder = new BufferingApplicationStartup(1000) + + GenericApplicationContext context + + void setup() { + context = new GenericApplicationContext() + context.applicationStartup = recorder + context.refresh() + } + + void cleanup() { + context.close() + } + + void 'an update reports each change set it runs as an item of a startup task'() { + given: + DataSource dataSource = newDatabase() + + when: 'the database is updated for the production context' + update(dataSource, 'dataSource') + + then: 'a task says what it does and how many change sets it will run, leaving out the test-only one' + StartupStep task = taskStep() + tags(task) == [(StartupTask.DESCRIPTION_TAG): 'Running database migrations', (StartupTask.TOTAL_TAG): '2'] + + and: 'each change set it ran is an item of the task, in the order they ran' + items(task) == ['create-book', 'create-author'] + + and: 'the database was updated' + tables(dataSource).containsAll(['BOOK', 'AUTHOR']) + !tables(dataSource).contains('SHELF') + } + + void 'an update with nothing left to run reports a task with no items'() { + given: 'a database already up to date' + DataSource dataSource = newDatabase() + update(dataSource, 'dataSource') + recorder.drainBufferedTimeline() + + when: + update(dataSource, 'dataSource') + + then: + StartupStep task = taskStep() + tags(task)[StartupTask.TOTAL_TAG] == '0' + items(task).empty + } + + void 'an update of another data source says which data source it updates'() { + when: + update(newDatabase(), 'dataSource_reports') + + then: + tags(taskStep())[StartupTask.DESCRIPTION_TAG] == 'Running database migrations on dataSource_reports' + } + + void 'an update nothing records reports nothing, and updates the database as before'() { + given: 'a context whose start nothing records' + GenericApplicationContext unrecorded = new GenericApplicationContext() + unrecorded.refresh() + DataSource dataSource = newDatabase() + + when: + update(dataSource, 'dataSource', unrecorded) + + then: 'no task is reported anywhere' + taskStep() == null Review Comment: `taskStep()` reads `recorder`, but the `unrecorded` context never records into it, so this can't fail. Deleting the `isRecorded` guard in `startTask` would leave the whole spec green. To make it bite, the callbacks could capture the listener in `onStartMigration` and assert that none was installed. Alternatively, count how often the change log is read. There's also no case with a failing change set. Moving `task?.close()` out of the `finally`, or dropping `runFailed`, would go unnoticed. The hibernate7 copy has the same gaps. ########## grails-test-examples/startup-progress/src/integration-test/groovy/startupprogress/StartupProgressPageSpec.groovy: ########## @@ -0,0 +1,107 @@ +/* + * 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 startupprogress + +import java.util.concurrent.TimeUnit + +import spock.lang.Specification + +import org.springframework.context.ConfigurableApplicationContext + +import grails.boot.GrailsApp + +/** + * Starts the application the way {@code bootRun} does, on a port of its own with the progress page turned + * on, and follows the start the way the progress page does: polling the status until it says the + * application is ready. + */ +class StartupProgressPageSpec extends Specification { + + private static final String STATUS_PATH = '/__grails/startup-progress' + + ConfigurableApplicationContext context + + void cleanup() { + context?.close() + } + + void 'a browser opening the application while it starts sees progress until BootStrap has run'() { + given: 'an application whose beans and BootStrap take their time' + int port = freePort() + Throwable failure = null + Thread runner = Thread.start('startup-progress-app') { + try { + context = GrailsApp.run(Application, + "--server.port=${port}".toString(), + '--grails.startup.progress.enabled=true', + '--startup.demo.delay.searchIndex=2s', + '--startup.demo.bootstrapDelay=3s') + } + catch (Throwable ex) { + failure = ex + } + } + + when: 'the status is polled until it says the application is ready' + Set<String> phases = new LinkedHashSet<>() + long deadline = System.nanoTime() + TimeUnit.MINUTES.toNanos(2) + while (!phases.contains('READY') && System.nanoTime() < deadline) { + Map response = get(port, STATUS_PATH) + if (response?.phase) { + phases << response.phase + } + Thread.sleep(50) + } + runner.join(TimeUnit.MINUTES.toMillis(1)) + + then: 'the page was told about the beans being created and about BootStrap running' + failure == null + phases.contains('CREATING_BEANS') + phases.contains('INITIALIZING') + + and: 'by the time the page was told the application is ready, BootStrap had finished' + phases.contains('READY') + get(port, '/').body.contains('BootStrap seeded 3 catalog items') Review Comment: This check runs after `runner.join(...)` on line 71. By then `GrailsApp.run` has returned and `BootStrap` has finished, whenever the page was told. So it would still pass if `READY` were reported right after the web server started, which is the regression this block's label describes. To prove the ordering, fetch `/` inside the loop at the moment the first `READY` status comes back, before the join, and assert the seeded text there. ########## grails-shell-cli/src/main/groovy/org/grails/cli/profile/commands/io/ServerInteraction.groovy: ########## @@ -47,17 +47,36 @@ trait ServerInteraction { } /** - * Returns true if the server is available + * Returns true if the server is available, which is once the application's web server answers on the port. + * An application that uses the {@code grails-startup-progress} module answers on the port before its web + * server starts, marking each response with the {@code Grails-Startup-Phase} header, so a response that + * carries it means the application is still starting. Once the web server has the port, it answers this + * request itself, even while {@code BootStrap} runs, as it does for an application without that module. * * @param host The host * @param port The port */ boolean isServerAvailable(String host = 'localhost', int port = 8080) { try { - new Socket(host, port) - return true + new Socket(host, port).close() } catch (e) { return false } + HttpURLConnection connection = null + try { + connection = (HttpURLConnection) URI.create("http://${host}:${port}/";).toURL().openConnection() + connection.requestMethod = 'HEAD' + connection.instanceFollowRedirects = false + connection.connectTimeout = 1000 + connection.readTimeout = 2000 + connection.responseCode + String phase = connection.getHeaderField('Grails-Startup-Phase') + return phase == null || phase == 'READY' + } catch (e) { Review Comment: A minor one: a `ConnectException` lands here too and is reported as available. The socket on line 61 can connect to the progress server just before it stops, either at the hand-off or when `failed()` releases the port. The `HEAD` is then refused and `run-app` stops waiting before the web server has the port. Catching `ConnectException` first and returning `false` would keep this fallback for the SSL case it's meant for. While you're here, `openConnection(Proxy.NO_PROXY)` on line 67 would stop a JVM-wide proxy setting from routing this localhost probe through a proxy. That would happen with an `http.nonProxyHosts` that doesn't list `localhost`, and the proxy's answer has no phase header. ########## grails-data-hibernate5/dbmigration/src/main/groovy/org/grails/plugins/databasemigration/liquibase/GrailsLiquibase.groovy: ########## @@ -82,25 +86,66 @@ class GrailsLiquibase extends SpringLiquibase { @Override protected void performUpdate(Liquibase liquibase) throws LiquibaseException { - if (!applicationContext.containsBean('migrationCallbacks')) { - super.performUpdate(liquibase) - return - } + // begun before the migration callbacks run, so a callback that sets a change listener of its own replaces the + // one that reports each change set, and works as it did before the update was reported + StartupTask task = startTask(liquibase) + try { + if (!applicationContext.containsBean('migrationCallbacks')) { + super.performUpdate(liquibase) + return + } + + def database = liquibase.database + def migrationCallbacks = applicationContext.getBean('migrationCallbacks') + + if (migrationCallbacks.metaClass.respondsTo(migrationCallbacks, 'beforeStartMigration')) { + migrationCallbacks.invokeMethod('beforeStartMigration', [database] as Object[]) + } + if (migrationCallbacks.metaClass.respondsTo(migrationCallbacks, 'onStartMigration')) { + migrationCallbacks.invokeMethod('onStartMigration', [database, liquibase, changeLog] as Object[]) + } - def database = liquibase.database - def migrationCallbacks = applicationContext.getBean('migrationCallbacks') + super.performUpdate(liquibase) - if (migrationCallbacks.metaClass.respondsTo(migrationCallbacks, 'beforeStartMigration')) { - migrationCallbacks.invokeMethod('beforeStartMigration', [database] as Object[]) + if (migrationCallbacks.metaClass.respondsTo(migrationCallbacks, 'afterMigrations')) { + migrationCallbacks.invokeMethod('afterMigrations', [database] as Object[]) + } } - if (migrationCallbacks.metaClass.respondsTo(migrationCallbacks, 'onStartMigration')) { - migrationCallbacks.invokeMethod('onStartMigration', [database, liquibase, changeLog] as Object[]) + finally { + task?.close() } + } - super.performUpdate(liquibase) + /** + * Begins a {@link StartupTask} for the update when anything records the application's start, such as the + * startup progress page, which then shows how many change sets are left and the one being run. Returns + * {@code null} when nothing records the start, since knowing how many change sets there are takes one more + * read of the change log and the database. + */ + private StartupTask startTask(Liquibase liquibase) { + if (!StartupTask.isRecorded(applicationContext)) { + return null + } + StartupTask task = StartupTask.start(applicationContext, migrationDescription(), pendingChangeSets(liquibase)) + liquibase.changeExecListener = new StartupTaskChangeExecListener(task) Review Comment: This replaces the listener instead of adding to it. That has two side effects whenever the start is recorded, which includes Actuator's `BufferingApplicationStartup` as well as the progress page: - **A callback's own listener knocks this one out.** If `migrationCallbacks.onStartMigration` sets its own listener (the case the comment on lines 89-90 describes), this listener is gone, but the task has already been tagged with the total. The signed-in page shows "0 of 2" for the whole update, and the report ends with "Running database migrations (0 of 2)" although both ran. `a change listener a migration callback sets of its own takes the place of the one that reports each change set` asserts only the total tag, so it doesn't catch this. - **A configured listener class is ignored.** Once a listener instance is passed, Liquibase 4.27's `ChangeExecListenerCommandStep` uses it and never builds the one configured with `liquibase.command.changeExecListenerClass`. That configured listener silently stops running whenever the start is recorded. `Liquibase` has no getter for the current listener. `createLiquibase` (line 59) could return a `Liquibase` that overrides `setChangeExecListener` and combines what it's given with the reporting listener, for example in a `DefaultChangeExecListener`. The callback's listener and the progress count would then both work. The hibernate7 copy has the same issue. ########## grails-doc/src/en/guide/gettingStarted/runningAndDebuggingAnApplication.adoc: ########## @@ -74,3 +74,189 @@ For debugging a Grails app, you have two options. You can either right-click on $ ./gradlew bootRun --debug-jvm For more information on the `bootRun` command, please refer to the link:{commandLineRef}bootRun.html[bootRun section of the Grails reference guide]. + +[[startupProgress]] +=== Watching the Application Start + +The `grails-startup-progress` module shows how far an application has got as it starts. Add it to the application's `build.gradle`, or select the `grails-startup-progress` feature when generating the application: + +[source,groovy] +---- +dependencies { + implementation 'org.apache.grails:grails-startup-progress' +} +---- + +With it, in development mode, which is the `development` environment run from the project directory as `./gradlew bootRun` does, Grails answers on the application's port as soon as the application begins to start, rather than leaving the port closed until the embedded server is ready. Opening the application in a browser while it starts shows a progress page with: + +* the stage the start has reached: preparing the application context, loading plugins and bean definitions, creating beans, starting the web server, and running plugin startup hooks and `BootStrap` classes +* how many of the application's beans have been created, and which bean is being created now +* the beans that have taken longest to create so far, not counting the beans they depend on, which is usually the quickest way to find out why a start is slow + +The page reloads the address it was opened at once the application is ready, which is after `BootStrap` has finished, not merely once the web server is listening. A browser that opens the application while `BootStrap` runs is shown the progress page too, rather than pages from an application that has not finished starting. + +Until the web server is listening, any request that is not for a web page, such as a call to a REST endpoint, gets a `503 Service Unavailable` response with a `Retry-After` header. Once it is listening, such requests reach the application as they always have, including requests the application makes to itself from `BootStrap`. + +If the start fails, the page shows the exception and its stack trace and stays open. Start the application again and the page follows the new start and reloads when it is ready. + +The stages, the beans, the exception and the Grails version are details of the application's internals, so the page shows them only to a browser signed in with the address the application logs as it starts: + +[source,console] +---- +Startup progress is shown at http://localhost:8080/?grailsStartupToken=… until the application is ready +---- + +Opening that address once signs the browser in for as long as the application runs, through restarts by Spring Boot DevTools, and takes the token back out of the address bar. A browser opened by the application, as described below, is signed in already. Anyone else reaching the port sees only the progress bar: the details are left out of the page and out of its data, rather than hidden in them. The sign-in is a cookie, so it covers every tab of the browser that signed in. The token is made afresh each time the JVM starts and is only ever written to the log, so seeing the details takes the same access as reading the log. + +Until the embedded server takes the port over, the page is served by the HTTP server built into the JDK, so it changes nothing about how the application itself serves requests. In the interactive shell, `grails run-app` keeps waiting while the page holds the port, and stops waiting once the embedded server has taken the port over, as it does for an application without the module, so it does not wait for `BootStrap`. The page is not served when the application: + +* is deployed as a WAR to a servlet container +* uses a random port (`server.port: 0`) or SSL +* finds its port already in use, in which case the application fails to start exactly as it would without the page + +These settings control the page: + +[source,yaml] +---- +grails: + startup: + progress: + enabled: false # whether to serve the page; by default, only in development mode Review Comment: Someone who copies this block just to set `openBrowser` turns the page, the details and the report off in development. These three settings have no default of `false`: left unset, development mode decides. Could `enabled`, `showDetails` and `endpoint.enabled` be shown commented out, as unset, with the comment explaining the default? -- 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]
