codeconsole commented on PR #16142: URL: https://github.com/apache/grails-core/pull/16142#issuecomment-5286467548
Both open items are now implemented — you'd said either was fine as a follow-up, so each is an isolated commit at the tip and can be dropped without touching the rest if you'd rather they wait. ### `(mtime, length)` pre-check — `12fed6255c` The stamp the source had when it last matched the checksum is remembered, and the read is skipped while neither it nor the length has moved. Steady state for a reload-enabled application is back to one stat per page per check interval. The comment at the call site spells out the role change you flagged: the timestamp is a fast path for skipping work, never the thing that decides staleness. Anything that moves it without changing the page — checkout, copy, touch — costs one hash and then correctly reports no change, where the old comparison reported the page stale. Coherence with the `CacheEntry` turned out to be simpler than I expected: the stamp is only consulted inside the callable the cache already gates, so there's one decision per interval rather than two layers racing. Covered by `a later check re-reads the source once its stamp moves, and skips the read while it has not`, which has to sleep past `grails.gsp.reload.interval` to reach a second check — the skip isn't otherwise observable through the public API. It asserts both halves: the equal-length, equal-mtime edit goes unseen (the documented trade-off), and a length change is caught. Removing the pre-check fails it. ### Checksum on the runtime-compile path — `b6a7c3f79e` One correction to the premise, which changed the implementation. `buildPageMetaInfo` doesn't hold anything checksummable — it decodes straight off the stream with `IOUtils.toString(inputStream, getGspEncoding())` and then runs the result through `decorateGroovyPageSource`. Checksumming either of those would produce a value that `establishChecksum`'s raw read at reload time can never match, since decode/re-encode doesn't have to round-trip. So it now buffers the raw bytes and decodes from the buffer, which keeps it a single read and leaves the checksum over the stored bytes. `IOUtils` became unused and is gone. `applyLastModifiedFromResource` still runs, so the timestamp fallback is untouched. New spec asserts both: the recorded checksum equals one taken over the raw bytes, and the modification time is still recorded. ### Also Full `:grails-gsp-core`, `:grails-gsp` and `:grails-web-gsp` test and check tasks are green. On the flaky `ScaffoldingFunctionalSpec` failure that showed on the earlier run — TestLens flagged it independently (❌ then ✅), and it passed on 17 and 25 while failing only on 21. It clicks an `<option>` element directly, which WebDriver doesn't reliably honour, so the required `Book.author` association fails validation and the page stays on `/book/create`; the `waitFor` then passes on a different condition than the assertion that follows requires. Unrelated to this PR and worth its own issue rather than a drive-by fix here. -- 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]
