codeconsole commented on code in PR #16142:
URL: https://github.com/apache/grails-core/pull/16142#discussion_r3779509126
##########
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:
Correction to my reply above — you were right and my reasoning wasn't. Fixed
in `7920a56816`.
I said one wrapper had to stay because `GroovyPageParser`'s constructor
takes an `InputStream`. That's true of the signature but not of the class: the
`InputStream` constructor is a thin delegate that calls `readStream` — which is
just `IOUtils.toString(in, encoding)` with a default — and forwards to the
`String` constructor with identical `expressionCodecName` and `ConfigMap`
semantics.
So the compiler can call that constructor directly. Both wrappers are now
gone, along with the `withStream` closure that had wrapped the whole compile
body, and it still reads the file once. It also removes a hop through an
overload that existed only to decode.
I checked the constructor's signature and not its body, which is how I
talked myself into a constraint that wasn't there.
`GroovyPageCompilerReproducibilitySpec` and `ParseSpec` re-run clean.
##########
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:
Retargeted to `7.1.x`.
##########
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`. 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`.
##########
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`. Remembers the `(mtime, length)` the source had
when it last matched, and skips the read while neither has moved.
##########
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:
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.
--
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]
