Make precompiled GSPs reproducible without disabling runtime reloading - #16142
Make precompiled GSPs reproducible without disabling runtime reloading#16142codeconsole wants to merge 13 commits into
Conversation
| // changed? -- and answers it identically on every machine. GroovyPageMetaInfo prefers it and | ||
| // falls back to LAST_MODIFIED for pages compiled by earlier versions, so the zero here means | ||
| // "no timestamp recorded", never "never reload". | ||
| gpp.lastModified = 0L |
There was a problem hiding this comment.
Isn't this a breaking change? I'm fine with it being removed long term, but setting this value here could cause downstream adopters to break. @matrei are you ok with such a change in 7.x?
There was a problem hiding this comment.
Retargeted to 7.1.x.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
The mechanism is sound and the test coverage is unusually thorough — the fallback matrix in GroovyPageMetaInfoReloadSpec covers the cases that matter. The inline comments are mostly about hardening edges: the -1 error sentinel caught by the new guard, cross-version behavior of LAST_MODIFIED = 0, steady-state I/O of the checksum check, and two test-hygiene items that could silently hollow out the new specs.
Two thoughts that don't attach to a diff line:
buildPageMetaInfoalready materializes the full source when compiling at runtime (GroovyPagesTemplateEngine.java:570), so recording a checksum on that path too would be nearly free and would unify the two staleness mechanisms — dev-mode edits inside the 2000 ms granularity window are exactly the misses the checksum was built to catch, and it is the precondition for ever retiring the timestamp branch. Fine as a follow-up.getLastModified()returning0for newly precompiled pages is disclosed in the description, but the getter itself carries no documentation of the new convention; a javadoc line there pointing atgetSourceChecksum()would cover external callers better than a release note alone.
| // granularity is required since lastmodified information is rounded somewhere in copying & war (zip) file information | ||
| // usually the lastmodified time is 1000L apart in files and in files extracted from the zip (war) file | ||
| if (currentLastmodified > 0 && Math.abs(currentLastmodified - lastModified) > LASTMODIFIED_CHECK_GRANULARITY) { | ||
| if (currentLastmodified > 0 && lastModified > 0 && |
There was a problem hiding this comment.
establishLastModified returns -1 when the resource's timestamp cannot be read (IOException/FileNotFoundException), and File.lastModified() returns 0 on I/O error — applyLastModifiedFromResource stores whichever it got. A runtime-compiled page (no checksum) that recorded such a value used to self-heal here: |current − (−1)| always exceeded the granularity, so the first check that could read a real mtime reloaded the page once and re-recorded a valid timestamp. With lastModified > 0, that page can never reload until restart, and the failure is silent.
The trigger is narrow (a transient I/O failure while the meta info is built, or a URL resource that reports no timestamp), but the precompiler's deliberate sentinel is only ever 0, so -1 can keep its old meaning — e.g. lastModified != 0 preserves the recovery path while still treating 0 as "nothing recorded".
There was a problem hiding this comment.
Fixed in 45e2452cb9. Guard is now lastModified != 0, so -1 keeps self-healing and only 0 means "nothing recorded". Added a spec that fails if reverted to > 0.
| // content answers the question directly: a page that was merely touched is not stale, and | ||
| // an edit is caught however close together the writes fall. | ||
| if (sourceChecksum != null) { | ||
| String currentChecksum = establishChecksum(resource); |
There was a problem hiding this comment.
Each staleness check for a checksum-bearing page now opens and fully reads the source to hash it, where the timestamp path cost one stat. It is gated per page per grails.gsp.reload.interval (5 s), but in the reload-enabled deployed scenario this feature targets — many views, sources on NFS/shared storage — that is a full file read per hot page per interval on request threads, indefinitely, even when nothing changes.
A cheap pre-check would restore stat-level steady-state cost: remember the (lastModified, contentLength) observed when the checksum was last computed and only re-hash when either moves. The only edit that slips through is one preserving both mtime and length — still strictly better than the old 2000 ms granularity window, and the spec's "edit within the granularity window" feature keeps passing since its edit changes the length. Not blocking, just worth weighing before this meets an app with hundreds of views.
There was a problem hiding this comment.
Implemented in 12fed6255c. Remembers the (mtime, length) the source had when it last matched, and skips the read while neither has moved.
| // changed? -- and answers it identically on every machine. GroovyPageMetaInfo prefers it and | ||
| // falls back to LAST_MODIFIED for pages compiled by earlier versions, so the zero here means | ||
| // "no timestamp recorded", never "never reload". | ||
| gpp.lastModified = 0L |
There was a problem hiding this comment.
A compatibility note worth capturing in the release notes: LAST_MODIFIED = 0 only means "nothing recorded" to a runtime that contains this change. A pre-7.0.16 grails-gsp reading a class precompiled by this compiler evaluates the old condition — |currentLastmodified − 0| > granularity, true for any real mtime — so with reload enabled every such page is declared stale on its first check and recompiled from source at runtime (e.g. a plugin precompiled with 7.0.16+ consumed by an app still resolving an older 7.0.x). Self-correcting and reload-only, but surprising when it hits.
Related: GroovyPageMetaInfo's constructor still reads LAST_MODIFIED unguarded (GroovyPageMetaInfo.java:132), unlike the null-guarded SOURCE_CHECKSUM read — so the constant is now permanent ABI that must keep being emitted even though its value is always 0. A sentence on the constant would keep a future cleanup from dropping it.
There was a problem hiding this comment.
Cross-version note added to the 7.0 → 7.1 upgrade guide. Comment added at the unguarded LAST_MODIFIED read explaining it is now permanent ABI.
| * @throws IOException if the source cannot be read | ||
| * @since 7.0.16 | ||
| */ | ||
| public static String checksumOf(InputStream source) throws IOException { |
There was a problem hiding this comment.
The javadoc already describes the parameter as "the raw bytes of the GSP source" — and both callers actually hold the bytes: GroovyPageCompiler reads gspfile.bytes and wraps them in a ByteArrayInputStream solely to fit this signature, and establishChecksum could use resource.getContentAsByteArray() (spring-core 6.0.5+). A checksumOf(byte[]) delegating to MessageDigest.digest(byte[]) would drop the manual read loop, both stream wrappers in the compiler, and the "closing a ByteArrayInputStream is a no-op" comment — and shrink the new public surface to something harder to misuse.
Whichever shape stays, it is worth spelling out that the input must be the raw stored bytes of the page — not the decorated/re-encoded source the runtime parse path works with — since that is the invariant that keeps a compile-time checksum comparable with establishChecksum's raw read at reload time.
There was a problem hiding this comment.
Done in e4055c2cb7 and 7920a56816. checksumOf(byte[]), and both stream wrappers are gone — the InputStream constructor just delegates to the String one, which I'd missed. Raw-bytes invariant is in the javadoc.
|
|
||
| GSP reloading is supported for precompiled GSPs since Grails 1.3.5. | ||
|
|
||
| A precompiled GSP records a checksum of the page it was compiled from, and is reloaded when the source no longer matches that checksum. Comparing content rather than modification times means a page that was copied or checked out afresh — and so carries a new modification time but the same content — is not needlessly recompiled, and an edit is detected however close together two writes fall. Pages compiled at runtime are still compared by modification time, and so is any page precompiled by Grails 7.0 or earlier. |
There was a problem hiding this comment.
This ships in a 7.0.x release, so "precompiled by Grails 7.0 or earlier" reads as including the very version that introduces the checksum. Suggest bounding it at the actual version:
| A precompiled GSP records a checksum of the page it was compiled from, and is reloaded when the source no longer matches that checksum. Comparing content rather than modification times means a page that was copied or checked out afresh — and so carries a new modification time but the same content — is not needlessly recompiled, and an edit is detected however close together two writes fall. Pages compiled at runtime are still compared by modification time, and so is any page precompiled by Grails 7.0 or earlier. | |
| A precompiled GSP records a checksum of the page it was compiled from, and is reloaded when the source no longer matches that checksum. Comparing content rather than modification times means a page that was copied or checked out afresh — and so carries a new modification time but the same content — is not needlessly recompiled, and an edit is detected however close together two writes fall. Pages compiled at runtime are still compared by modification time, and so is any page precompiled by a version of Grails earlier than 7.0.16. |
There was a problem hiding this comment.
Applied, bounded at 7.1.6.
|
|
||
| void 'a source compiled at two different modification times produces identical classes'() { | ||
| when: 'the same page is compiled twice, as two checkouts of one commit would' | ||
| this.page.setLastModified(1_000_000_000_000L) |
There was a problem hiding this comment.
File.setLastModified() fails silently by returning false on filesystems that reject explicit mtime changes (container overlay FS, some CI volumes). If that happens here and at line 57, both compilations see the same mtime and first == second passes vacuously — the exact regression this spec exists to catch would ship undetected. Asserting the return value makes the environment problem loud instead:
| this.page.setLastModified(1_000_000_000_000L) | |
| assert this.page.setLastModified(1_000_000_000_000L) |
(same at line 57)
There was a problem hiding this comment.
Applied at both sites.
| ClassLoader loader = new URLClassLoader([targetDir.toURI().toURL()] as URL[], getClass().classLoader) | ||
| new GroovyPageMetaInfo(loader.loadClass(results.values().first() as String)) |
There was a problem hiding this comment.
URLClassLoader is Closeable, and each call here leaks one loader (with its open file handles) for the life of the test JVM. On Windows those handles can block @TempDir cleanup, failing an otherwise green spec. Both uses of the loader complete before the closure returns, so it can be closed immediately:
| ClassLoader loader = new URLClassLoader([targetDir.toURI().toURL()] as URL[], getClass().classLoader) | |
| new GroovyPageMetaInfo(loader.loadClass(results.values().first() as String)) | |
| new URLClassLoader([targetDir.toURI().toURL()] as URL[], getClass().classLoader).withCloseable { URLClassLoader loader -> | |
| new GroovyPageMetaInfo(loader.loadClass(results.values().first() as String)) | |
| } |
There was a problem hiding this comment.
Applied — now withCloseable.
| Resource resource = sourcePage() | ||
| GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo() | ||
| metaInfo.sourceChecksum = checksumOf(resource) | ||
| resource.getFile().setLastModified(resource.getFile().lastModified() + 86_400_000L) |
There was a problem hiding this comment.
Same setLastModified() caveat as in the reproducibility spec, but here it erodes the premise rather than the assertion: this feature (and the pins back to originalTimestamp at lines 113 and 126) still pass via the checksum path if the call silently returns false, but then they no longer exercise the scenario their names document — "touched but not edited" runs with an unmoved mtime, and "edit within the granularity window" runs with an mtime that moved well past the granularity, which the old timestamp path already caught. An assert on all three keeps the coverage honest.
There was a problem hiding this comment.
Applied at all three sites.
ac9a423 to
855003d
Compare
|
@codeconsole Can you give me the TLDR of why this is going into 7 and not 8?
How would this not be going into a patch version? |
|
Why 7 and not 8: the bug is shipping from 7.x now — jars carrying precompiled GSPs don't reproduce, and Why not a patch: it adds public API — If you're fine with the API addition on a patch branch, 7.0.x works and I'll move it back. |
@codeconsole I don't understand, if I'm not totally mistaken, this would land in 7.1.6, which is a patch release. At this point, I think it would be wise to put this in 8. |
|
@matrei the original target was 7.0.x. because of the pushback, I just bumped it on a less iterated version. I think it should go into 7.0.x, but I really don't care as long as it gets into 8.0.x. Not having it is obviously causing a lot of build issues, so if we are going to keep doing releases for 7.x, it would be a good idea to get it in there unless you think it is breaking. Where do you recommend we put it? |
@jdaugherty Wasn't
@codeconsole 8.0.0 |
GroovyPageCompiler baked the .gsp source file's modification time into every generated page class. GroovyPageParser emits that value as a `static final long LAST_MODIFIED` constant, so it forms part of the compiled class's ABI. A fresh checkout gives every .gsp a new modification time, so identical sources compiled on two machines produce different bytes. Because the divergence is ABI-level it is not hidden by classpath normalization: consumers re-key even under COMPILE_CLASSPATH, which otherwise ignores everything but the ABI. Any jar bundling precompiled GSPs therefore invalidates the build cache for every downstream task on every fresh checkout. Archive reproducibility was not the gap -- the jars already use normalized entry timestamps. Emit a fixed LAST_MODIFIED so precompilation is reproducible. Verified by compiling the same sources with only the mtime changed: before: 3 distinct mtimes -> 3 distinct sets of class bytes after: 3 distinct mtimes -> byte-identical output A sibling closure class from the same task, carrying no LAST_MODIFIED constant, was byte-identical in every run both before and after, confirming the timestamp is the sole source of divergence. The value is read at runtime, so fixing it is not sufficient on its own. GroovyPageMetaInfo.checkIfReloadableResourceHasChanged compares the field against the live source timestamp to decide whether a precompiled page is stale; a fixed value would make that comparison always report a change. Guard it so that a lastModified of 0 means "no source timestamp recorded" and staleness detection is skipped rather than firing on every check. Behaviour for pages carrying a real timestamp is unchanged. For GSPs in binary plugin jars the reload path was already unreachable -- DefaultGroovyPageLocator.resolveViewInBinaryPlugin nulls the resource callable, and those jars ship no .gsp sources. The guard covers an application's own precompiled pages with reloading enabled. The LAST_MODIFIED field is retained rather than removed because GroovyPageMetaInfo resolves it reflectively via findField. Fixes apache#16131
…n time Precompiled GSPs baked the .gsp source's modification time into the generated class as a LAST_MODIFIED constant. Git stores no modification times, so every fresh clone or CI checkout gave each source a new one and byte-for-byte identical sources compiled to different classes. Because the value was a compile-time constant it belonged to the class's ABI and was inlined into callers, so the difference survived even Gradle's COMPILE_CLASSPATH normalization and every task downstream of a jar carrying precompiled GSPs missed the build cache. GroovyPageParser now also emits a SOURCE_CHECKSUM of the page source, and GroovyPageCompiler emits LAST_MODIFIED as 0, so identical sources compile to identical bytes on every machine. Runtime reloading of precompiled pages -- documented since Grails 1.3.5, and reached for an application's own pages whenever grails.gsp.enable.reload is set -- is preserved rather than dropped. GroovyPageMetaInfo compares the recorded checksum against the live source, falling back to the timestamp for pages compiled by earlier versions. Comparing content is also more accurate than the timestamp it replaces: a page that was merely copied or checked out afresh is no longer treated as changed, and an edit is caught however close together two writes fall. Fixes apache#16131
The two signals can disagree: a page can carry a matching checksum beside a stale timestamp, or an edited source whose timestamp did not move. Pin down that the checksum decides in both directions.
establishLastModified returns -1 when a resource's timestamp cannot be read at all, and applyLastModifiedFromResource stores that as-is. Such a page used to recover on its own: the difference against -1 always exceeded the granularity, so the first check able to read a real mtime reloaded it once and recorded a valid timestamp. Guarding with lastModified > 0 swept -1 up with the compiler's deliberate 0 and stranded those pages until restart, silently. Only 0 means "nothing recorded", so test for that exactly.
Both callers already hold the bytes -- the compiler reads gspfile.bytes, and the runtime can use Resource.getContentAsByteArray() -- so the stream signature only forced a ByteArrayInputStream wrapper and a manual read loop. Digesting a byte[] drops both and leaves a smaller public method that is harder to misuse. Also records the invariant the checksum depends on: the input must be the raw stored bytes of the page, not the decoded source the parse path works with.
Adding public API rules this out of a patch release, so it moves to the minor branch where additive API is allowed. 7.1.x also has an upgrade guide covering 7.0 to 7.1, which gives the getLastModified() behaviour change somewhere to be announced -- 7.0.x had no within-7.0 guide to put it in. Records the cross-version behaviour there too: an application on an earlier Grails consuming a plugin precompiled by 7.1.6 evaluates the old condition against LAST_MODIFIED = 0 and declares every such page stale on first check. Reload-only and self-correcting, but surprising when mixing versions.
Hashing means reading the page in full, so a reload-enabled application with many views paid a read per hot page per check interval, indefinitely, even when nothing changed -- where the timestamp comparison it replaced cost one stat. Remember the modification time and length the source had when it last matched the recorded checksum, and skip the read while neither has moved. The timestamp is a fast path for skipping work here, never the thing that decides staleness: anything that moves it without changing the page costs one hash and then correctly reports no change. The one edit this misses preserves both the modification time and the exact length, a narrower gap than the grails.gsp.reload.granularity window it replaces.
A page compiled at runtime recorded only a modification time, so the two staleness mechanisms disagreed: precompiled pages compared content while runtime-compiled ones compared timestamps and kept missing edits inside the grails.gsp.reload.granularity window. In development that is the case that matters most -- two saves in quick succession. buildPageMetaInfo already materializes the whole source, so this costs nothing beyond buffering the bytes before decoding rather than after. It must checksum the stored bytes rather than gspSource or the decorated source, since the runtime re-reads the resource raw when comparing. The timestamp is still recorded, so nothing about the fallback path changes.
Recording a checksum for pages compiled at runtime made the guide sentence and the fallback-branch comment wrong: both still said runtime-compiled pages are compared by modification time. Only pages precompiled before SOURCE_CHECKSUM existed reach that branch now.
Recording a checksum for pages compiled at runtime changes reloading in both directions and the upgrade note covered only precompiled pages. An edit is now caught whenever the modification time or length moves, where a save landing inside grails.gsp.reload.granularity used to go unnoticed; and a page whose timestamp moves without its content changing is no longer recompiled, so touch no longer forces a reload.
The InputStream constructor is a thin delegate: it calls readStream, which is IOUtils.toString with an encoding default, and forwards to the String constructor with the same expressionCodec semantics. Calling that constructor directly drops the ByteArrayInputStream and the closure wrapping the whole compile body, still reads the file once, and removes a hop through an overload that existed only to decode.
matrei asked for this on 8.0.x. Rebased there, upgrade note moved from the 7.0 -> 7.1 guide into upgrading80x, and the version references that bound the checksum behaviour changed from 7.1.6 to 8.0.0.
7920a56 to
2335f9b
Compare
|
@matrei review the original PR this built on top of #16132
I moved to |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16142 +/- ##
==================================================
+ Coverage 52.3242% 52.6897% +0.3656%
- Complexity 18537 18573 +36
==================================================
Files 2039 2037 -2
Lines 97498 97055 -443
Branches 17138 17004 -134
==================================================
+ Hits 51015 51138 +123
+ Misses 38998 38421 -577
- Partials 7485 7496 +11
🚀 New features to boost your workflow:
|
|
@matrei I think the AI is overstating the reproducible problem. I think with the SOURCE_EPOCH settings technically the build is reproducible (which is why it works for us). I think this is meant to address a build problem - where the gradle cache is never hit due to unique gsp files. This seems like a real issue. I'm ok with this going into 8, but if it does, should we even keep the last modified date? It seems like a checksum is a better value to keep instead of a date? Then it's cache stable always. |
|
@jdaugherty correct, the word reproducibility is being used in a different context here and no the same context we are using to verify builds. |
✅ All tests passed ✅🏷️ Commit: 6a620eb Learn more about TestLens at testlens.app. |
6 participants
Builds on @maczikasz's commit from #16132, preserved at the base of this branch. That commit diagnosed the problem and fixed the reproducibility half; the checksum layer on top is what keeps runtime reloading working.
The problem
GroovyPageCompilerbaked the.gspsource's modification time into every generated class as aLAST_MODIFIEDconstant. Git stores no modification times, so every fresh clone or CI checkout gives each source a new one, and byte-for-byte identical sources compile to different classes.Because the value is a compile-time constant it belongs to the class's ABI and is inlined into callers, so the difference survives even Gradle's
COMPILE_CLASSPATHnormalization. Every task downstream of a jar carrying precompiled GSPs misses the build cache — #16132 measured roughly 616 hours of avoidable CI task re-execution over one week.Those jars also don't reproduce.
etc/bin/verify-reproducible.shrebuilds published artifacts and diffs them, with no GSP exclusion, so a verifier's checkout yields different bytes than the release builder's forgrails-fields,grails-spring-security, and anything else shipping precompiled pages.Why the timestamp couldn't simply be zeroed
LAST_MODIFIEDis read at runtime.GroovyPageMetaInfo.checkIfReloadableResourceHasChangedcompares it against the live source to decide whether a precompiled page is stale, andDefaultGroovyPageLocatorinstalls a resource callable for any precompiled page when reloading is enabled — only the binary-plugin path nulls it.Zeroing the constant alone switches off reloading for an application's own precompiled pages, which is documented behaviour ("GSP reloading is supported for precompiled GSPs since Grails 1.3.5"), and it fails silently — the page renders stale with nothing logged.
The change
GroovyPageParseremits aSOURCE_CHECKSUMrecording what the source is rather than when it was touched.GroovyPageMetaInfoprefers it, falling back toLAST_MODIFIEDfor pages compiled by earlier versions. Identical sources now compile to identical bytes on every machine.Reload detection gets more accurate than the timestamp it replaces: a page copied or checked out afresh is no longer treated as changed, and an edit is caught however close together two writes fall.
(mtime, length)stamp is remembered when the source last matched, and the read is skipped while neither has moved — steady state stays at one stat per page per check interval.0from-1. Only0means "nothing recorded";-1is whatestablishLastModifiedyields on an unreadable timestamp, and those pages must keep self-healing.Branch
Targets
8.0.xper @matrei. It adds public API —GroovyPageParser.checksumOf, plusgetSourceChecksum/setSourceChecksum— which is what ruled out a patch branch.checksumOfcan't be avoided: the compiler andGroovyPageMetaInfoare in different packages and must not disagree on how a source is digested.Notes
getLastModified()returns0for pages precompiled by 8.0.0+. Documented on the getter and in the Grails 8 upgrade guide;getSourceChecksum()is the replacement.GroovyPageCompiler's own up-to-date check at line 214 is untouched.GroovyPageForkCompileTaskis a@CacheableTaskwith content-hashed inputs, so that check is unreachable for anything built through Gradle.The doc change also corrects the documented default for
grails.gsp.reload.granularity(table said 1000, code says 2000).Fixes #16131