From 3a3aed6ead2c46ccbb0bbe00c75a4541bd1a8307 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Sun, 1 Oct 2017 11:15:40 +0100 Subject: [PATCH] ResourceServlet: Consistently use class load time as mtime The GWT UI static servlets (e.g. WarGwtUiServlet) were already using class load time as the "mtime" of a file, to account for the fact that the build system always sets the mtime of zipfile entries to the Unix epoch. (This is intentional, to keep bazel builds reproducible.) PolyGerrit has the same problem served from a WAR file, but was not applying this hack. Complicating the situation is that the same class (PolyGerritUiServlet) is used for the dev server, where we can trust the mtime. Hackily check whether the path in question is on-disk in the implementation of getLastModifiedTime. There was also a bug in ResourceServlet#maybeStream that was bypassing getLastModifiedTime when checking If-Modified-Since; fix that as well. While we're in here, fix the other broken class in this hierarchy, WarDocServlet. In retrospect, it may have been a mistake to make ResourceServlet try to handle all possible caching behaviors, since it makes bugs like this all too easy to introduce. Long term, may want to disentangle the on-disk implementation, which may depend on mtime to determine staleness, from the zipfile implementation, which shouldn't. Bug: Issue 6885 Change-Id: Iea041774c1005cb9918c462d01aef05cff61da23 (cherry picked from commit 65c8345a96c4de35ccf4ddc09a6bc74ce6a429f8) --- .../gerrit/httpd/raw/PolyGerritUiServlet.java | 18 ++++++++++++++++++ .../gerrit/httpd/raw/ResourceServlet.java | 3 +-- .../google/gerrit/httpd/raw/WarDocServlet.java | 11 +++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/PolyGerritUiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/PolyGerritUiServlet.java index 2f3d32fefc..c508b2d8be 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/PolyGerritUiServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/PolyGerritUiServlet.java @@ -15,11 +15,17 @@ package com.google.gerrit.httpd.raw; import com.google.common.cache.Cache; +import com.google.gerrit.common.TimeUtil; +import java.io.IOException; +import java.nio.file.FileSystems; import java.nio.file.Path; +import java.nio.file.attribute.FileTime; class PolyGerritUiServlet extends ResourceServlet { private static final long serialVersionUID = 1L; + private static final FileTime NOW = FileTime.fromMillis(TimeUtil.nowMs()); + private final Path ui; PolyGerritUiServlet(Cache cache, Path ui) { @@ -31,4 +37,16 @@ class PolyGerritUiServlet extends ResourceServlet { protected Path getResourcePath(String pathInfo) { return ui.resolve(pathInfo); } + + @Override + protected FileTime getLastModifiedTime(Path p) throws IOException { + if (ui.getFileSystem().equals(FileSystems.getDefault())) { + // Assets are being served from disk, so we can trust the mtime. + return super.getLastModifiedTime(p); + } + // Assume this FileSystem is serving from a WAR. All WAR outputs from the build process have + // mtimes of 1980/1/1, so we can't trust it, and return the initialization time of this class + // instead. + return NOW; + } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java index c79fa747da..6b37bdb8d9 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java @@ -31,7 +31,6 @@ import com.google.common.base.CharMatcher; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableMap; import com.google.common.hash.Hashing; -import com.google.gerrit.common.FileUtil; import com.google.gerrit.common.Nullable; import com.google.gerrit.httpd.HtmlDomUtil; import com.google.gwtexpui.server.CacheHeaders; @@ -252,7 +251,7 @@ public abstract class ResourceServlet extends HttpServlet { return true; } - long lastModified = FileUtil.lastModified(p); + long lastModified = getLastModifiedTime(p).toMillis(); if (req.getDateHeader(IF_MODIFIED_SINCE) >= lastModified) { rsp.setStatus(SC_NOT_MODIFIED); return true; diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/WarDocServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/WarDocServlet.java index 93bd5aed1d..3f6ff25653 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/WarDocServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/WarDocServlet.java @@ -15,12 +15,16 @@ package com.google.gerrit.httpd.raw; import com.google.common.cache.Cache; +import com.google.gerrit.common.TimeUtil; import java.nio.file.FileSystem; import java.nio.file.Path; +import java.nio.file.attribute.FileTime; class WarDocServlet extends ResourceServlet { private static final long serialVersionUID = 1L; + private static final FileTime NOW = FileTime.fromMillis(TimeUtil.nowMs()); + private final FileSystem warFs; WarDocServlet(Cache cache, FileSystem warFs) { @@ -32,4 +36,11 @@ class WarDocServlet extends ResourceServlet { protected Path getResourcePath(String pathInfo) { return warFs.getPath("/Documentation/" + pathInfo); } + + @Override + protected FileTime getLastModifiedTime(Path p) { + // Return initialization time of this class, since the WAR outputs from the build process all + // have mtimes of 1980/1/1. + return NOW; + } }