Repository: vert-x3/vertx-web
Issue: StaticHandlerImpl uses a shared DateFormat across threads causing issues with date parsing in header values

Continuing on this curious issue in Quarkus https://github.com/quarkusio/quarkus/issues/4627, apart from the fix done yesterday here https://github.com/vert-x3/vertx-web/issues/1423, it would be really odd that Firefox (that too the latest version) would send incorrect values for a header like `If-Modified-Since`. The user attached the browser's request header screenshots here https://github.com/quarkusio/quarkus/issues/4627#issuecomment-543247193 and they show that Firefox is sending the right header values. So looking back into the code in `StaticHandlerImpl`, I think what's happening is multiple requests (across threads) are being handled by it and it uses a single instance of `DateFormat` to parse the header value. The `DateFormat` (`java.text.SimpleDateFormat`) is created once for that handler and stored as a member variable. The javadoc of `SimpleDateFormat` states:
```
* Date formats are not synchronized.
 * It is recommended to create separate format instances for each thread.
 * If multiple threads access a format concurrently, it must be synchronized
 * externally.
```
So what seems to be happening in that issue that is that its running into a race condition due to multiple threads using the same DateFormatter.
I think the implementation in this `StaticHandlerImpl` should either create the DateFormat as and when needed or maybe have a `ThreadLocal`.

Changed files:
vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java, vertx-web/src/main/java/io/vertx/ext/web/impl/Utils.java, vertx-web/src/test/java/io/vertx/ext/web/handler/SessionHandlerTestBase.java, vertx-web/src/test/java/io/vertx/ext/web/handler/CookieHandlerTest.java, vertx-web/src/main/java/io/vertx/ext/web/handler/impl/LoggerHandlerImpl.java, vertx-web/src/test/java/io/vertx/ext/web/handler/StaticHandlerTest.java

diff --git a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/LoggerHandlerImpl.java b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/LoggerHandlerImpl.java
index a76a6513e..282e8e99a 100755
--- a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/LoggerHandlerImpl.java
+++ b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/LoggerHandlerImpl.java
@@ -27,9 +27,6 @@ import io.vertx.ext.web.handler.LoggerHandler;
 import io.vertx.ext.web.RoutingContext;
 import io.vertx.ext.web.impl.Utils;
 
-import java.text.DateFormat;
-import java.util.Date;
-
 /** # Logger
  *
  * Logger for request. There are 3 formats included:
@@ -48,10 +45,6 @@ public class LoggerHandlerImpl implements LoggerHandler {
 
   private final io.vertx.core.logging.Logger logger = LoggerFactory.getLogger(this.getClass());
 
-  /** The Date formatter (UTC JS compatible format)
-   */
-  private final DateFormat dateTimeFormat = Utils.createRFC1123DateTimeFormatter();
-
   /** log before request or after
    */
   private final boolean immediate;
@@ -119,7 +112,7 @@ public class LoggerHandlerImpl implements LoggerHandler {
 
         message = String.format("%s - - [%s] \\"%s %s %s\\" %d %d \\"%s\\" \\"%s\\"",
           remoteClient,
-          dateTimeFormat.format(new Date(timestamp)),
+          Utils.formatRFC1123DateTime(timestamp),
           method,
           uri,
           versionFormatted,
diff --git a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java
index b85df7d5c..14e34dd26 100644
--- a/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java
+++ b/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/StaticHandlerImpl.java
@@ -34,8 +34,6 @@ import io.vertx.ext.web.impl.Utils;
 
 import java.io.File;
 import java.nio.charset.Charset;
-import java.text.DateFormat;
-import java.text.ParseException;
 import java.util.*;
 import java.util.concurrent.Callable;
 import java.util.regex.Matcher;
@@ -54,7 +52,6 @@ public class StaticHandlerImpl implements StaticHandler {
 
   private static final Logger log = LoggerFactory.getLogger(StaticHandlerImpl.class);
 
-  private final DateFormat dateTimeFormatter = Utils.createRFC1123DateTimeFormatter();
   private String webRoot = DEFAULT_WEB_ROOT;
   private long maxAgeSeconds = DEFAULT_MAX_AGE_SECONDS; // One day
   private boolean directoryListing = DEFAULT_DIRECTORY_LISTING;
@@ -112,7 +109,7 @@ public class StaticHandlerImpl implements StaticHandler {
       // We use cache-control and last-modified
       // We *do not use* etags and expires (since they do the same thing - redundant)
       Utils.addToMapIfAbsent(headers, "cache-control", "public, max-age=" + maxAgeSeconds);
-      Utils.addToMapIfAbsent(headers, "last-modified", dateTimeFormatter.format(props.lastModifiedTime()));
+      Utils.addToMapIfAbsent(headers, "last-modified", Utils.formatRFC1123DateTime(props.lastModifiedTime()));
       // We send the vary header (for intermediate caches)
       // (assumes that most will turn on compression when using static handler)
       if (sendVaryHeader && request.headers().contains("accept-encoding")) {
@@ -121,7 +118,7 @@ public class StaticHandlerImpl implements StaticHandler {
     }
 
     // date header is mandatory
-    headers.set("date", dateTimeFormatter.format(new Date()));
+    headers.set("date", Utils.formatRFC1123DateTime(System.currentTimeMillis()));
   }
 
   @Override
@@ -742,14 +739,8 @@ public class StaticHandlerImpl implements StaticHandler {
         // Not a conditional request
         return false;
       }
-      Date ifModifiedSinceDate;
-      try {
-        ifModifiedSinceDate = dateTimeFormatter.parse(ifModifiedSince);
-      } catch (ParseException e) {
-        // Behave like the header is not present
-        return false;
-      }
-      boolean modifiedSince = Utils.secondsFactor(props.lastModifiedTime()) > ifModifiedSinceDate.getTime();
+      long ifModifiedSinceDate = Utils.parseRFC1123DateTime(ifModifiedSince);
+      boolean modifiedSince = Utils.secondsFactor(props.lastModifiedTime()) > ifModifiedSinceDate;
       return !modifiedSince;
     }
 
diff --git a/vertx-web/src/main/java/io/vertx/ext/web/impl/Utils.java b/vertx-web/src/main/java/io/vertx/ext/web/impl/Utils.java
index 47ff09536..c2dff873a 100644
--- a/vertx-web/src/main/java/io/vertx/ext/web/impl/Utils.java
+++ b/vertx-web/src/main/java/io/vertx/ext/web/impl/Utils.java
@@ -32,6 +32,12 @@ import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
 import java.util.Locale;
 import java.util.TimeZone;
 
@@ -91,10 +97,19 @@ public class Utils extends io.vertx.core.impl.Utils {
     }
   }
 
-  public static DateFormat createRFC1123DateTimeFormatter() {
-    DateFormat dtf = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss zzz", Locale.ENGLISH);
-    dtf.setTimeZone(TimeZone.getTimeZone("GMT"));
-    return dtf;
+  private static final ZoneId ZONE_GMT = ZoneId.of("GMT");
+
+  public static String formatRFC1123DateTime(final long time) {
+    return DateTimeFormatter.RFC_1123_DATE_TIME.format(Instant.ofEpochMilli(time).atZone(ZONE_GMT));
+  }
+
+  public static long parseRFC1123DateTime(final String header) {
+    try {
+      return header == null || header.isEmpty() ? -1 :
+        LocalDateTime.parse(header, DateTimeFormatter.RFC_1123_DATE_TIME).toInstant(ZoneOffset.UTC).toEpochMilli();
+    } catch (DateTimeParseException ex) {
+      return -1;
+    }
   }
 
   public static String pathOffset(String path, RoutingContext context) {
diff --git a/vertx-web/src/test/java/io/vertx/ext/web/handler/CookieHandlerTest.java b/vertx-web/src/test/java/io/vertx/ext/web/handler/CookieHandlerTest.java
index 7a2d3dd07..710fa3e14 100644
--- a/vertx-web/src/test/java/io/vertx/ext/web/handler/CookieHandlerTest.java
+++ b/vertx-web/src/test/java/io/vertx/ext/web/handler/CookieHandlerTest.java
@@ -22,7 +22,6 @@ import io.vertx.ext.web.impl.Utils;
 import io.vertx.ext.web.WebTestBase;
 import org.junit.Test;
 
-import java.text.DateFormat;
 import java.util.*;
 
 /**
@@ -137,8 +136,8 @@ public class CookieHandlerTest extends WebTestBase {
     int startPos = encoded.indexOf("Expires=");
     int endPos = encoded.indexOf(';', startPos);
     String expiresDate = encoded.substring(startPos + 8, endPos);
-    Date d = dateTimeFormat.parse(expiresDate);
-    assertTrue(d.getTime() - now >= maxAge);
+    long d = Utils.parseRFC1123DateTime(expiresDate);
+    assertTrue(d - now >= maxAge);
 
     cookie.setMaxAge(Long.MIN_VALUE);
     cookie.setSecure(true);
@@ -146,6 +145,4 @@ public class CookieHandlerTest extends WebTestBase {
     cookie.setHttpOnly(true);
     assertEquals("foo=bar; Path=/somepath; Domain=foo.com; Secure; HTTPOnly", cookie.encode());
   }
-
-  private final DateFormat dateTimeFormat = Utils.createRFC1123DateTimeFormatter();
 }
diff --git a/vertx-web/src/test/java/io/vertx/ext/web/handler/SessionHandlerTestBase.java b/vertx-web/src/test/java/io/vertx/ext/web/handler/SessionHandlerTestBase.java
index 375d48ee5..2134a80dd 100644
--- a/vertx-web/src/test/java/io/vertx/ext/web/handler/SessionHandlerTestBase.java
+++ b/vertx-web/src/test/java/io/vertx/ext/web/handler/SessionHandlerTestBase.java
@@ -19,13 +19,11 @@ package io.vertx.ext.web.handler;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.ext.web.Session;
 import io.vertx.ext.web.WebTestBase;
-import io.vertx.ext.web.impl.Utils;
 import io.vertx.ext.web.sstore.AbstractSession;
 import io.vertx.ext.web.sstore.LocalSessionStore;
 import io.vertx.ext.web.sstore.SessionStore;
 import org.junit.Test;
 
-import java.text.DateFormat;
 import java.util.Objects;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
@@ -360,8 +358,6 @@ public abstract class SessionHandlerTestBase extends WebTestBase {
 				resp -> assertNull(resp.headers().get("set-cookie")), 200, "OK", null);
 	}
 
-	private final DateFormat dateTimeFormatter = Utils.createRFC1123DateTimeFormatter();
-
 	protected long doTestSessionRetryTimeout() throws Exception {
 		router.route().handler(SessionHandler.create(store));
 		AtomicReference<Session> rid = new AtomicReference<>();
diff --git a/vertx-web/src/test/java/io/vertx/ext/web/handler/StaticHandlerTest.java b/vertx-web/src/test/java/io/vertx/ext/web/handler/StaticHandlerTest.java
index da95bbee3..e10672e45 100644
--- a/vertx-web/src/test/java/io/vertx/ext/web/handler/StaticHandlerTest.java
+++ b/vertx-web/src/test/java/io/vertx/ext/web/handler/StaticHandlerTest.java
@@ -16,7 +16,6 @@
 
 package io.vertx.ext.web.handler;
 
-import io.vertx.core.Handler;
 import io.vertx.core.http.*;
 import io.vertx.core.json.JsonArray;
 import io.vertx.core.net.PemKeyCertOptions;
@@ -29,7 +28,6 @@ import org.junit.Test;
 
 import java.io.File;
 import java.nio.file.Files;
-import java.text.DateFormat;
 import java.util.*;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicReference;
@@ -40,8 +38,6 @@ import java.util.function.BiConsumer;
  */
 public class StaticHandlerTest extends WebTestBase {
 
-  private final DateFormat dateTimeFormatter = Utils.createRFC1123DateTimeFormatter();
-
   protected StaticHandler stat;
 
   @Override
@@ -307,7 +303,7 @@ public class StaticHandlerTest extends WebTestBase {
 
   @Test
   public void testCacheGetNew() throws Exception {
-    testCacheReturnFromCache((lastModified, req) -> req.putHeader("if-modified-since", dateTimeFormatter.format(toDateTime(lastModified) - 1)), 200, "OK", "<html><body>Other page</body></html>");
+    testCacheReturnFromCache((lastModified, req) -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(toDateTime(lastModified) - 1)), 200, "OK", "<html><body>Other page</body></html>");
   }
 
   @Test
@@ -442,7 +438,7 @@ public class StaticHandlerTest extends WebTestBase {
       String lastModified = res.headers().get("last-modified");
       assertEquals(modified, toDateTime(lastModified));
     }, 200, "OK", "<html><body>File system page</body></html>");
-    testRequest(HttpMethod.GET, "/fspage.html", req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified)), null, 304, "Not Modified", null);
+    testRequest(HttpMethod.GET, "/fspage.html", req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified)), null, 304, "Not Modified", null);
   }
 
   @Test
@@ -458,7 +454,7 @@ public class StaticHandlerTest extends WebTestBase {
       resource.setLastModified(modified + 1000);
     }, 200, "OK", "<html><body>File system page</body></html>");
     // But it should still return not modified as the entry is cached
-    testRequest(HttpMethod.GET, "/fspage.html", req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified)), null, 304, "Not Modified", null);
+    testRequest(HttpMethod.GET, "/fspage.html", req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified)), null, 304, "Not Modified", null);
   }
 
   @Test
@@ -481,7 +477,7 @@ public class StaticHandlerTest extends WebTestBase {
     }, 200, "OK", html);
     // But it should return a new entry as the entry is now old
     Thread.sleep(cacheEntryTimeout + 1);
-    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified)), res -> {
+    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified)), res -> {
       String lastModified = res.headers().get("last-modified");
       assertEquals(modified + 1000, toDateTime(lastModified));
     }, 200, "OK", html);
@@ -489,7 +485,7 @@ public class StaticHandlerTest extends WebTestBase {
     // 304 must still work when cacheEntry.isOutOfDate() == true, https://github.com/vert-x3/vertx-web/issues/726
     Thread.sleep(cacheEntryTimeout + 1);
 
-    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified + 1000)), 304, "Not Modified", null);
+    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified + 1000)), 304, "Not Modified", null);
   }
 
   @Test
@@ -506,10 +502,10 @@ public class StaticHandlerTest extends WebTestBase {
     stat.setCacheEntryTimeout(3600 * 1000);
 
     long modified = Utils.secondsFactor(pageFile.lastModified());
-    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified)), null, 304, "Not Modified", null);
+    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified)), null, 304, "Not Modified", null);
     pageFile.delete();
     testRequest(HttpMethod.GET, page, 404, "Not Found");
-    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", dateTimeFormatter.format(modified)), null, 404, "Not Found", null);
+    testRequest(HttpMethod.GET, page, req -> req.putHeader("if-modified-since", Utils.formatRFC1123DateTime(modified)), null, 404, "Not Found", null);
 
   }
 
@@ -855,8 +851,7 @@ public class StaticHandlerTest extends WebTestBase {
 
   private long toDateTime(String header) {
     try {
-      Date date = dateTimeFormatter.parse(header);
-      return date.getTime();
+      return Utils.parseRFC1123DateTime(header);
     } catch (Exception e) {
       fail(e.getMessage());
       return -1;