codeconsole commented on code in PR #16142:
URL: https://github.com/apache/grails-core/pull/16142#discussion_r3779215841
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageCompiler.groovy:
##########
@@ -215,11 +215,26 @@ class GroovyPageCompiler {
File gspgroovyfile = new File(new
File(generatedGroovyPagesDirectory, packageDir), className + '.groovy')
// gspgroovyfile.getParentFile().mkdirs()
- gspfile.withInputStream { InputStream gspinput ->
+ byte[] gspSource = gspfile.bytes
+ // closing a ByteArrayInputStream is a no-op, so there is nothing
to release here
+ String sourceChecksum = GroovyPageParser.checksumOf(new
ByteArrayInputStream(gspSource))
+
+ new ByteArrayInputStream(gspSource).withStream { InputStream
gspinput ->
GroovyPageParser gpp = new GroovyPageParser(viewuri - '.gsp',
viewuri, gspfile.absolutePath, gspinput, encoding, expressionCodec, configMap)
gpp.packageName = packageName
gpp.className = className
- gpp.lastModified = gspfile.lastModified()
+ // Record what the source *is*, not when it was last touched.
LAST_MODIFIED is emitted as a
+ // `static final long`, so it belongs to the class's ABI and
is inlined into callers, which
+ // even Gradle's COMPILE_CLASSPATH normalization cannot see
past. Git stores no modification
+ // times, so every checkout gave each .gsp a new one and
identical sources compiled to
+ // different bytes, costing every downstream consumer of the
jar its build cache.
+ //
+ // SOURCE_CHECKSUM answers what the timestamp was only ever a
proxy for -- has the source
+ // 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
Review Comment:
You're right, and I've moved it rather than argue the point — this now
targets `7.1.x`, rebased, with @maczikasz's commit still at the base.
Concretely what changes: `GroovyPageMetaInfo.getLastModified()` returns `0`
for pages precompiled by this version instead of the source mtime. Nothing in
this repository reads the getter — the field's only consumer is the staleness
check right below it, which this PR rewires — but it is public and I can't see
downstream.
The two halves can't be separated, which is why there's no smaller version
of this: the reproducibility fix *is* dropping the timestamp from the class,
and dropping it without recording something in its place silently disables
reloading for an application's own precompiled pages.
The move paid for itself twice, as it turns out: `7.1.x` has an upgrade
guide covering 7.0 → 7.1, so the `getLastModified()` change now has somewhere
to be announced. `7.0.x` had no within-7.0 guide, which is why I'd fallen back
to "release note" earlier.
Happy to move it back if you and @matrei would rather have it in `7.0.x` —
the unreproducible jars are shipping from there today, and
`etc/bin/verify-reproducible.sh` has no GSP exclusion, so release verification
currently diffs on every artifact carrying precompiled pages. Your call.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPageMetaInfo.java:
##########
@@ -474,10 +518,21 @@ public Resource checkIfReloadableResourceHasChanged(final
PrivilegedAction<Resou
public Resource call() {
Resource resource = resourceCallable.run();
if (resource != null && resource.exists()) {
+ // A page compiled by GroovyPageCompiler records a
checksum of its source rather than the
+ // source's modification time, which git does not preserve
across a checkout. Comparing
+ // 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);
+ return (currentChecksum != null &&
!sourceChecksum.equals(currentChecksum)) ? resource : null;
+ }
+ // Pages compiled before SOURCE_CHECKSUM existed, and
pages compiled at runtime, still carry a
+ // real timestamp. A lastModified of 0 means none was
recorded, so there is nothing to compare.
long currentLastmodified = establishLastModified(resource);
// 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 &&
Review Comment:
Fixed in `45e2452cb9`. Good catch — this was a real regression, not a nit.
Confirmed the path: `establishLastModified` returns `-1` for a null
resource, `FileNotFoundException` and `IOException`, and
`applyLastModifiedFromResource` stores it verbatim. Under `lastModified > 0`
such a page could never reload again, silently, exactly as you describe.
Guard is now `lastModified != 0`, so `0` keeps its "nothing recorded"
meaning while `-1` keeps its self-healing one. Added `a page whose timestamp
could not be established reloads once and recovers`, and verified it's a real
guard rather than decoration: reverting to `> 0` fails that feature alone and
leaves the others passing.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPageMetaInfo.java:
##########
@@ -474,10 +518,21 @@ public Resource checkIfReloadableResourceHasChanged(final
PrivilegedAction<Resou
public Resource call() {
Resource resource = resourceCallable.run();
if (resource != null && resource.exists()) {
+ // A page compiled by GroovyPageCompiler records a
checksum of its source rather than the
+ // source's modification time, which git does not preserve
across a checkout. Comparing
+ // 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);
Review Comment:
Implemented in `12fed6255c` — you were right that this needed weighing
before it met an app with hundreds of views.
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.
Coherence with the `CacheEntry` turned out 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. The comment at the
call site spells out the role change — the timestamp is a fast path for
skipping work here, never the thing that decides staleness, so anything that
moves it without changing the page costs one hash and then correctly reports no
change.
Covered by `a later check re-reads the source once its stamp moves, and
skips the read while it has not`. It has to sleep past
`grails.gsp.reload.interval` to reach a second check, since the skip isn't
otherwise observable through the public API — flagging that as a test smell you
may want to weigh in on. 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.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageCompiler.groovy:
##########
@@ -215,11 +215,26 @@ class GroovyPageCompiler {
File gspgroovyfile = new File(new
File(generatedGroovyPagesDirectory, packageDir), className + '.groovy')
// gspgroovyfile.getParentFile().mkdirs()
- gspfile.withInputStream { InputStream gspinput ->
+ byte[] gspSource = gspfile.bytes
+ // closing a ByteArrayInputStream is a no-op, so there is nothing
to release here
+ String sourceChecksum = GroovyPageParser.checksumOf(new
ByteArrayInputStream(gspSource))
+
+ new ByteArrayInputStream(gspSource).withStream { InputStream
gspinput ->
GroovyPageParser gpp = new GroovyPageParser(viewuri - '.gsp',
viewuri, gspfile.absolutePath, gspinput, encoding, expressionCodec, configMap)
gpp.packageName = packageName
gpp.className = className
- gpp.lastModified = gspfile.lastModified()
+ // Record what the source *is*, not when it was last touched.
LAST_MODIFIED is emitted as a
+ // `static final long`, so it belongs to the class's ABI and
is inlined into callers, which
+ // even Gradle's COMPILE_CLASSPATH normalization cannot see
past. Git stores no modification
+ // times, so every checkout gave each .gsp a new one and
identical sources compiled to
+ // different bytes, costing every downstream consumer of the
jar its build cache.
+ //
+ // SOURCE_CHECKSUM answers what the timestamp was only ever a
proxy for -- has the source
+ // 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
Review Comment:
Both taken.
The cross-version note is now in the 7.0 → 7.1 upgrade guide rather than
release notes, since retargeting to `7.1.x` gave it a proper home. One
narrowing from tracing it: for GSPs inside binary plugin jars
`DefaultGroovyPageLocator.resolveViewInBinaryPlugin` nulls the resource
callable, so those never reach the comparison at all. The reachable case is the
one you describe — an app whose own `gsp/views.properties` is absent, so
`getResource('classpath:gsp/views.properties')` picks up a plugin's and the
plugin's pages get a live callable. Narrow, self-correcting, reload-only, and
genuinely surprising when it lands.
On the unguarded read: correct, and there's now a comment at
`GroovyPageMetaInfo.java:132` recording that `LAST_MODIFIED` is permanent ABI —
every compiler version has emitted it, so a page class without it can't exist,
and it must keep being emitted even though the value is always `0`, because
pages precompiled by earlier versions still carry a real timestamp and are
still compared by it.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:
##########
@@ -1380,6 +1391,54 @@ public void setLastModified(long lastModified) {
this.lastModified = lastModified;
}
+ /**
+ * Computes the checksum recorded in the {@code SOURCE_CHECKSUM} constant
of a generated page.
+ * <p>
+ * Both the compiler that writes the constant and the runtime that
compares against it use this method, so
+ * that the two can never disagree on how a GSP source is digested. The
stream is read to the end but is
+ * not closed; closing it remains the caller's responsibility.
+ *
+ * @param source the raw bytes of the GSP source
+ * @return the checksum as a lower-case hex string
+ * @throws IOException if the source cannot be read
+ * @since 7.0.16
+ */
+ public static String checksumOf(InputStream source) throws IOException {
Review Comment:
Adopted in `e4055c2cb7`. You're right that the stream signature bought
nothing — both callers hold the bytes already.
`checksumOf(byte[])` now delegates to `MessageDigest.digest(byte[])`, which
drops the read loop, the `IOException` from the signature, and the compiler's
checksum-specific `ByteArrayInputStream` along with its comment.
`establishChecksum` uses `resource.getContentAsByteArray()` (spring-core
resolves to 6.2.19 here, well past 6.0.5).
One wrapper does remain, so I don't want to claim otherwise:
`GroovyPageParser`'s constructor takes an `InputStream`, so the compiler still
wraps the bytes once to feed the parse. Keeping it avoids reading the file
twice.
The invariant is now spelled out in the javadoc — raw stored bytes, not the
decoded or re-encoded source the parse path works with. That turned out to
matter more than documentation: it's what forced the runtime-compile-path
checksum in `b6a7c3f79e` to buffer the bytes before decoding rather than hash
what `buildPageMetaInfo` already had in hand.
##########
grails-doc/src/en/guide/theWebLayer/gsp/makingChangesToADeployedApplication.adoc:
##########
@@ -46,8 +46,10 @@ There are also some system properties to control GSP
reloading:
|===
|Name|Description|Default
|grails.gsp.enable.reload|system property for enabling the GSP reload mode
(alternative to adding it in the file-based application configuration|
-|grails.gsp.reload.interval|interval between checking the lastmodified time of
the gsp source file, unit is milliseconds|5000
-|grails.gsp.reload.granularity|the number of milliseconds leeway to give
before deciding a file is out of date. this is needed because different
roundings usually cause a 1000ms difference in lastmodified times|1000
+|grails.gsp.reload.interval|interval between checks of the gsp source file,
unit is milliseconds|5000
+|grails.gsp.reload.granularity|the number of milliseconds leeway to give
before deciding a file is out of date. this is needed because different
roundings usually cause a 1000ms difference in lastmodified times. Applies only
to pages compared by modification time — see below|2000
|===
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.
Review Comment:
Applied. You're right that "Grails 7.0 or earlier" reads as including the
release that introduces the checksum — it's now bounded at 7.1.6, following the
retarget to `7.1.x`.
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/compiler/GroovyPageCompilerReproducibilitySpec.groovy:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.gsp.compiler
+
+import spock.lang.Specification
+import spock.lang.TempDir
+
+import org.grails.gsp.GroovyPageMetaInfo
+
+/**
+ * Precompiled GSPs must not vary with the modification time of their source.
+ *
+ * Git records no modification times, so every fresh clone or CI checkout
gives each .gsp a new one. Baking
+ * that into the generated class made otherwise identical jars differ on every
checkout, and because
+ * {@code LAST_MODIFIED} was a compile-time constant the difference survived
Gradle's compile-classpath
+ * normalization, so every downstream task missed the build cache.
+ */
+class GroovyPageCompilerReproducibilitySpec extends Specification {
+
+ private static final String PAGE_CONTENT = '<html><body><g:if
test="${flag}">Hello</g:if></body></html>'
+
+ @TempDir
+ File tempDir
+
+ private File viewsDir
+ private File page
+
+ void setup() {
+ this.viewsDir = new File(this.tempDir, 'views')
+ this.page = new File(this.viewsDir, 'index.gsp')
+ this.page.parentFile.mkdirs()
+ this.page.text = PAGE_CONTENT
+ }
+
+ 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)
Review Comment:
Applied at both call sites.
This one would have been genuinely nasty: a silent `false` makes `first ==
second` pass vacuously, so the spec would have gone green while the exact
regression it exists to catch shipped.
--
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]
