borinquenkid commented on code in PR #16032:
URL: https://github.com/apache/grails-core/pull/16032#discussion_r3694123764
##########
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:
Done — the `when:` label now reads `"the application is stopped"`, and
there's a comment noting `30_000` matches the value `stop-app.groovy:31` passes
in production, rather than framing it as an arbitrary generous number. Kept the
underscore style consistent with the `3_600_000L` literals already in the file.
Left the `DEFAULT_STOP_TIMEOUT_MILLIS` hoist out per your "fine to skip" note,
to 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:
You were right — the 15s→30s `stop()` budget wasn't the fix. Replaced the
tail `process.waitFor(10, TimeUnit.SECONDS)` with a `PollingConditions` wait on
the OS-backed liveness check, same idea as your suggested alternatives.
One more round worth flagging: my first pass at that polled
`process.isAlive()` (`java.lang.Process`), reasoning it was OS-backed like
`stop()`'s own fallback. Checked the JDK 21 source to confirm and that's wrong
— `Process#isAlive()` is `!hasExited`, set only when the same async
`ProcessHandleImpl` reaper-thread completion that backs `waitFor()`/`onExit()`
resolves. It's not a live query, so polling it would've just re-widened the
same budget from 10s to 30s rather than removing the dependency you flagged.
Fixed to poll `process.toHandle().isAlive()` instead, which delegates to
`ProcessHandleImpl`'s native `isAlive0()` — a genuine per-call OS check,
matching what `awaitExit()`'s fallback actually relies on. That version is
pushed now.
--
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]
