jdaugherty commented on code in PR #16142:
URL: https://github.com/apache/grails-core/pull/16142#discussion_r3775617129
##########
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:
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.
##########
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:
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:
```suggestion
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.
```
##########
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:
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.
##########
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:
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.
##########
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:
`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".
##########
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:
`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:
```suggestion
assert this.page.setLastModified(1_000_000_000_000L)
```
(same at line 57)
##########
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)
+ byte[] first = compileToBytes('first')
+
+ and:
+ this.page.setLastModified(1_700_000_000_000L)
+ byte[] second = compileToBytes('second')
+
+ then: 'the jar built from them is byte-identical, so downstream tasks
keep their cache hits'
+ first == second
+ }
+
+ void 'a compiled page records a checksum of its source instead of a
modification time'() {
+ when:
+ GroovyPageMetaInfo metaInfo = compileToMetaInfo('recorded')
+
+ then: 'the checksum identifies the content'
+ metaInfo.sourceChecksum ==~ /[0-9a-f]{64}/
+
+ and: 'no modification time is baked in for a fresh checkout to
invalidate'
+ metaInfo.lastModified == 0L
+ }
+
+ void 'an edited source produces a different checksum'() {
+ given:
+ GroovyPageMetaInfo before = compileToMetaInfo('before')
+
+ when:
+ this.page.text = '<html><body>something else entirely</body></html>'
+ GroovyPageMetaInfo after = compileToMetaInfo('after')
+
+ then: 'the runtime can still tell that the page changed'
+ before.sourceChecksum != after.sourceChecksum
+ }
+
+ private Map compile(File targetDir) {
+ targetDir.mkdirs()
+ GroovyPageCompiler compiler = new GroovyPageCompiler()
+ compiler.viewsDir = this.viewsDir
+ compiler.srcFiles = [this.page]
+ compiler.targetDir = targetDir
+ compiler.generatedGroovyPagesDirectory = new File(this.tempDir,
'generated').tap { mkdirs() }
+ compiler.compile()
+ }
+
+ private byte[] compileToBytes(String name) {
+ File targetDir = new File(this.tempDir, name)
+ Map results = compile(targetDir)
+ new File(targetDir, "${results.values().first()}.class").bytes
+ }
+
+ private GroovyPageMetaInfo compileToMetaInfo(String name) {
+ File targetDir = new File(this.tempDir, name)
+ Map results = compile(targetDir)
+ ClassLoader loader = new URLClassLoader([targetDir.toURI().toURL()] as
URL[], getClass().classLoader)
+ new GroovyPageMetaInfo(loader.loadClass(results.values().first() as
String))
Review 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:
```suggestion
new URLClassLoader([targetDir.toURI().toURL()] as URL[],
getClass().classLoader).withCloseable { URLClassLoader loader ->
new GroovyPageMetaInfo(loader.loadClass(results.values().first()
as String))
}
```
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/GroovyPageMetaInfoReloadSpec.groovy:
##########
@@ -0,0 +1,180 @@
+/*
+ * 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
+
+import java.security.PrivilegedAction
+
+import spock.lang.Specification
+import spock.lang.TempDir
+
+import org.springframework.core.io.FileSystemResource
+import org.springframework.core.io.Resource
+
+import org.grails.gsp.compiler.GroovyPageParser
+
+/**
+ * Reload-staleness behaviour of {@link GroovyPageMetaInfo}.
+ *
+ * A page compiled by {@code GroovyPageCompiler} records a checksum of its
source rather than the source's
+ * modification time, which git does not preserve across a checkout. Staleness
is therefore decided by
+ * comparing content, falling back to the timestamp for pages compiled before
the checksum existed.
+ *
+ * Each feature uses a fresh {@code GroovyPageMetaInfo}, because the result of
a check is cached for
+ * {@code grails.gsp.reload.interval} milliseconds.
+ */
+class GroovyPageMetaInfoReloadSpec extends Specification {
+
+ private static final String PAGE_CONTENT = '<html><body>hi</body></html>'
+
+ @TempDir
+ File tempDir
+
+ private Resource sourcePage(String content = PAGE_CONTENT) {
+ File page = new File(this.tempDir, 'index.gsp')
+ page.text = content
+ new FileSystemResource(page)
+ }
+
+ private static String checksumOf(Resource resource) {
+ resource.inputStream.withStream { InputStream input ->
GroovyPageParser.checksumOf(input) }
+ }
+
+ private static PrivilegedAction<Resource> callableFor(Resource resource) {
+ { -> resource } as PrivilegedAction
+ }
+
+ void 'a page whose recorded checksum matches its source is not reported as
stale'() {
+ given: 'a precompiled page recording the checksum of the source on
disk'
+ Resource resource = sourcePage()
+ GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo()
+ metaInfo.sourceChecksum = checksumOf(resource)
+
+ expect:
+ !metaInfo.shouldReload(callableFor(resource))
+ }
+
+ void 'a page whose source no longer matches its recorded checksum is
reported as stale'() {
+ given: 'a precompiled page whose source has since been edited'
+ Resource resource = sourcePage()
+ GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo()
+ metaInfo.sourceChecksum = checksumOf(resource)
+ resource.getFile().text = '<html><body>edited</body></html>'
+
+ expect:
+ metaInfo.shouldReload(callableFor(resource))
+ }
+
+ void 'a source that was touched but not edited is not reported as stale'()
{
+ given: 'a page whose source carries a modification time nothing like
the one it was compiled at'
+ Resource resource = sourcePage()
+ GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo()
+ metaInfo.sourceChecksum = checksumOf(resource)
+ resource.getFile().setLastModified(resource.getFile().lastModified() +
86_400_000L)
Review 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.
--
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]
