jamesfredley commented on PR #15698:
URL: https://github.com/apache/grails-core/pull/15698#issuecomment-4733279237
Pushed 18dd33505e addressing the latest review round (thanks @codeconsole,
@jdaugherty, @matrei). Summary of what changed and how each finding was handled.
## What changed in this commit
- **Producer-side test** - new `GrailsAppPidFileSpec` proves `GrailsApp`
registers Spring Boot's `ApplicationPidFileWriter` **only** when
`grails.cli.pid.file` is set (and not when it is absent/blank). Extracted
`GrailsApp.configureCliPidFileWriter()` to make this directly testable.
- **End-to-end producer coverage** - the TestKit specs now assert the
**exact** resolved path equals `<projectDir>/build/run-app.pid`, and a new
`grails-web` test project (`boot-run-pid-web`) proves `bootRun` supplies the
PID arg for a generated-style web app, not just `grails-app`.
- **Recycled-PID guard tests** - added `isRunning`/`stop` tests for
`RunningApplicationProcess` that backdate the PID file so the live process
started after it, exercising `isProbablyReusedPid`.
- **Marker cleanup** - `run-app.stopping` is now an empty, presence-only
file (it previously stored an unused `System.currentTimeMillis()`), with
comments clarifying it is purely a foreground message classifier, unrelated to
the graceful `ProcessHandle.destroy()` shutdown.
- **Docs** - moved the `--host`/`--port` removal into a dedicated Grails 7
upgrade note (`12.27 stop-app no longer uses JMX or the Actuator shutdown
endpoint`) that also documents the Windows best-effort shutdown, the
`build/run-app.pid` behavior, and the default-build-directory constraint. The
command reference was trimmed accordingly.
Verified locally (all green):
- `:grails-core:test --tests grails.boot.GrailsAppPidFileSpec`
- `:grails-shell-cli:test --tests
org.grails.cli.gradle.RunningApplicationProcessSpec`
- `:grails-gradle-plugins:test --tests
org.grails.gradle.plugin.core.GrailsGradlePluginToolchainSpec`
- `:grails-profiles-base:compileProfile`
## Response to the review findings
**1 - End-to-end loop (blocking).** The wiring is now proven by automated
tests on the producer side: `bootRun` supplies
`-Dgrails.cli.pid.file=<build>/run-app.pid` for both `grails-app` and
`grails-web` apps, and `GrailsApp` registers the writer when that property is
present. `org.apache.grails.gradle.grails-web` extends `GrailsGradlePlugin`,
which applies the Spring Boot plugin programmatically before
`configureBootRunPidFile`, so `withPlugin('org.springframework.boot')` always
fires. Spring Boot's `ApplicationPidFileWriter` writes on
`ApplicationPreparedEvent`, which fires during a normal startup. @matrei - your
earlier transcript was on the pre-rework commit; would you mind re-running on
Linux against this commit? Repro: `create-app` -> `cd app` -> `grails run-app`,
confirm `build/run-app.pid` appears, then `grails stop-app` from the same or a
separate terminal.
**2 - build-dir mismatch.** Resolved as a documented constraint (the
pragmatic call rather than having the CLI parse Gradle state). The producer
uses `project.layout.buildDirectory` and the consumer uses
`BuildSettings.TARGET_DIR` (`<projectDir>/build`); these coincide for the
**default** build directory, and customizing the Gradle build directory is
explicitly called out as unsupported in the plugin comment and the upgrade note.
**3 - already-running guard is racy.** Documented as best-effort in
`run-app.groovy`; with the default fixed port a second `run-app` simply fails
on a port conflict.
**4 - Windows not graceful.** Documented in both `stop-app.adoc` and the
upgrade note: Linux/macOS shut down gracefully (SIGTERM), Windows is
best-effort and graceful shutdown is not guaranteed.
**5 - interactive marker / async path.** Confirmed and documented: in
interactive mode `run-app` returns after the server is up, so the marker only
helps a **foreground** `grails run-app`. The marker comments now say exactly
this. Fully suppressing the background build-failure line in interactive mode
would require a Tooling-API build cancellation tied to `stop-app` - i.e. the
in-memory registry that was removed earlier at @jdaugherty's request - so I
left it out by design; the app is genuinely stopped and the line is cosmetic.
**6 - test gaps.** Added the highest-value ones: the `GrailsApp`
producer-side registration test, the recycled-PID guard, and
exact-path/`grails-web` TestKit coverage. `STILL_RUNNING`/`destroyForcibly` are
intentionally not unit-tested - a process that survives both SIGTERM and
SIGKILL within the timeout cannot be simulated portably. The `stop-app`
`--host`/`--port` fail-fast and the `run-app` marker branch live in the profile
command scripts (CLI integration surface); the underlying
`RunningApplicationProcess` they delegate to is covered.
**7 - polish.** Dead timestamp removed (empty marker). The marker is
deliberately **not** cleared on `STOPPED`: a foreground `run-app` blocked on
the build may not have observed the terminated process yet, so clearing it
there would race that check - it is cleared at the start of the next `run-app`
instead (now commented). The "every `bootRun` writes `build/run-app.pid`"
behavior change and the `--host`/`--port` removal are both in the upgrade note.
## On the dev-Actuator alternative (@codeconsole)
I kept the pure-PID approach rather than enabling Actuator/JMX in dev. It is
what @jdaugherty asked for, it works for forked processes and for `grails
stop-app` run from a separate invocation/terminal without enabling a shutdown
endpoint, and it avoids depending on attach/management-agent behavior. The
Windows graceful-shutdown gap is the one place the Actuator route would have
been better cross-platform; that limitation is now documented rather than
blocking.
--
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]
