GzipHandler fails to set Vary header on 304 responses when client does not accept gzip encoding

[markslater]

Jetty version(s)
11.0.13

Java version/vendor (use: java -version)
openjdk version "11.0.17" 2022-10-18
OpenJDK Runtime Environment (build 11.0.17+8-post-Ubuntu-1ubuntu220.04)
OpenJDK 64-Bit Server VM (build 11.0.17+8-post-Ubuntu-1ubuntu220.04, mixed mode, sharing)

OS type/version
Linux 5.4.0-120-generic

Description
#8906 improves the handling of 304 responses by the GzipHandler by setting the vary header on 304 responses that would have had that same header set on a 200 response. It works by identifying that an ETag sent by the client has a signature implying it was generated by the GzipHandler in the first place. However (and I feel a little guilty here as the implementation was my suggestion in #8905), this doesn't work when GzipHandler returns an uncompressed response due to a client not accepting gzip encoding. In this case, GzipHandler correctly adds a vary header to a 200 response, but because the resultant ETag has no identifying marks added to it, a subsequent 304 response doesn't have the vary header set.

Unfortunately, I didn't realise this until I tried 11.0.13 today :(

How to reproduce?

Use the following class:

package com.example;

import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.gzip.GzipHandler;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;

import java.io.IOException;
import java.io.Writer;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.net.http.HttpResponse.BodyHandlers;
import java.util.UUID;

public class GzipETagServer {
    public static void main(String[] args) throws Exception {
        final Server server = new Server(8080);
        final ServletContextHandler servletContextHandler = new ServletContextHandler();
        servletContextHandler.addServlet(new ServletHolder(new FooServlet()), "/foo");
        final GzipHandler gzipHandler = new GzipHandler();
        gzipHandler.setHandler(servletContextHandler);
        server.setHandler(gzipHandler);
        server.start();
        try {
            final HttpResponse<Void> httpResponse = HttpClient.newHttpClient().send(
                    HttpRequest.newBuilder(new URI("http://localhost:8080/foo")).build(),
                    BodyHandlers.discarding()
            );
            final String etag = httpResponse.headers().firstValue("etag").get();
            System.out.println("httpResponse.headers().allValues(\"vary\") = " + httpResponse.headers().allValues("vary"));
            final HttpResponse<Void> notModifiedResponse = HttpClient.newHttpClient().send(
                    HttpRequest.newBuilder(new URI("http://localhost:8080/foo")).setHeader("if-none-match", etag).build(),
                    BodyHandlers.discarding()
            );
            System.out.println("notModifiedResponse.headers().allValues(\"vary\") = " + notModifiedResponse.headers().allValues("vary"));
        } finally {
            server.stop();
        }
    }

    private static final class FooServlet extends HttpServlet {

        private final String eTag = '"' + UUID.randomUUID().toString() + '"';

        @Override
        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
            resp.setHeader("etag", eTag);
            if (eTag.equals(req.getHeader("if-none-match"))) {
                resp.setStatus(304);
            } else {
                resp.setStatus(200);
                try (Writer responseWriter = resp.getWriter()) {
                    responseWriter.write("Foo".repeat(100));
                }
            }
        }
    }
}

Outputs:

httpResponse.headers().allValues("vary") = [Accept-Encoding]
notModifiedResponse.headers().allValues("vary") = []

[joakime]

The request etag header (or related etag-ish headers) doesn't seem to be relevant to having a response with Vary: Accept-Encoding

We can put Vary: Accept-Encoding on 100% of responses from the server and neither Firefox nor Chrome seem to care.

What specific User Agent scenario are you dealing with?

[markslater]

In this case Jetty is serving a REST API, so the user agents are a mixture of browsers and various languages' HTTP clients. Adding to the mix, I've got a Varnish Cache reverse proxy in front of Jetty.

The problem with adding vary: accept-encoding on every response is that it decreases cacheability. With a wide variety of clients, we see several values of accept-encoding. If vary: accept-encoding is set on resources that can't be compressed, we're likely to get requests that could have been served from the cache going to the server due to their accept-encoding header, and identical copies of resources bloating the cache (at the expense of evicting valid resources). Inconveniently, some of the mime types that can't be compressed tend to be large, such as pngs and jpegs.

I agree with you that the etag header doesn't have any direct link to this; the current implementation leans on an artefact of GzipHandler's implementation to infer a value for the vary header. Ideally the header would be entirely accurate, though I'm not certain this is possible for the GzipHandler because 304 responses don't contain a content-type header.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[sbordet]

I think this issue may be related to #11210, where the logic to add the Vary header has been changed.

[joakime]

Issue #8907 also shows another possible GzipHandler issue