jdaugherty commented on code in PR #16032:
URL: https://github.com/apache/grails-core/pull/16032#discussion_r3652270114
##########
grails-shell-cli/src/test/groovy/org/grails/cli/gradle/RunningApplicationProcessSpec.groovy:
##########
@@ -208,8 +208,8 @@ class RunningApplicationProcessSpec extends Specification {
expect:
RunningApplicationProcess.isRunning(pidFile)
- when:
- def result = RunningApplicationProcess.stop(pidFile, 15000)
+ when: "a generous timeout budget gives headroom for
reaper-notification lag on a contended CI runner"
Review Comment:
The block description explains why the number is large rather than what the
stimulus is, so the Spock report ends up reading `when: a generous timeout
budget gives headroom for reaper-notification lag on a contended CI runner` for
a step whose stimulus is simply "the application is stopped". It also
undersells the change: `30000` is not an arbitrary generous value, it is
exactly what the shipped `stop-app` passes
(`grails-profiles/base/commands/stop-app.groovy:31`). Saying that makes this an
alignment with production instead of a number we grew until the test stopped
failing.
```groovy
when: "the application is stopped"
// Same budget the shipped stop-app command passes
(grails-profiles/base/commands/stop-app.groovy)
def result = RunningApplicationProcess.stop(pidFile, 30_000)
```
Also `30_000` for consistency with the `3_600_000L` literals already in this
file. If you want to go further, hoisting the value onto
`RunningApplicationProcess` (`static final long DEFAULT_STOP_TIMEOUT_MILLIS =
30_000L`) and using it from both `stop-app.groovy` and here would leave one
source of truth instead of two copies of the same magic number - fine to skip
if you would rather keep the diff minimal.
##########
grails-shell-cli/src/test/groovy/org/grails/cli/gradle/RunningApplicationProcessSpec.groovy:
##########
@@ -208,8 +208,8 @@ class RunningApplicationProcessSpec extends Specification {
expect:
RunningApplicationProcess.isRunning(pidFile)
- when:
- def result = RunningApplicationProcess.stop(pidFile, 15000)
+ when: "a generous timeout budget gives headroom for
reaper-notification lag on a contended CI runner"
+ def result = RunningApplicationProcess.stop(pidFile, 30000)
Review Comment:
This is the part I would like confirmed before merge.
`stop()` can only return `STILL_RUNNING` if `awaitExit()` returns false
twice: once after the `timeoutMillis` wait on `onExit()`, and again 5s after
`destroyForcibly()` (note `Math.min(timeoutMillis, 5000L)` - the second budget
does not move with this change, so the effective total goes 20s -> 35s). Both
of those returns fall through to `!process.isAlive()`, a direct OS liveness
check. For the target here - `sleep 60` / `ping -n 60 127.0.0.1`, hit with
`destroy()` and then `destroyForcibly()` - the OS still reporting it alive ~20s
later is hard to credit. `NOT_RUNNING` is ruled out by the `expect:
isRunning(pidFile)` immediately above. So I do not think the 15s budget is what
produced the failure.
The candidate I would check first is the tail condition seven lines down,
`process.waitFor(10, TimeUnit.SECONDS)`. It returns `false` - a failing Spock
condition - when the test JVM's own bookkeeping for its child has not caught up
within 10s, which is precisely the reaper-notification lag the description
blames, and this PR leaves its 10s budget untouched. That tail is also what
6c76333ea9/a13c38c3fd were fixing. If that is the condition failing on CI, this
change will not stop the flake and the fix belongs there instead - an unbounded
`process.waitFor()`, or a `PollingConditions` wait on `!process.isAlive()`.
Worth ruling out as well: `stop()` discards the boolean returned by
`destroy()`/`destroyForcibly()`, so a signal that never landed is
indistinguishable from a slow shutdown. Not something to fix in this PR, but it
is the other way `STILL_RUNNING` happens without any timeout being too short.
The Spock output from one flaky run settles which of these it is, since it
prints the rendered condition and the actual `StopResult`.
--
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]
