Modifying of HTTP headers in HttpChannel.Listener#onResponseBegin is no longer possible with Jetty 10

[uschindler]

Jetty version(s)
Jetty 10.0.8

Description
I asked this already on the mailing list:

I have a Jetty server that was upgraded from 9.4 to 10.0. There is one requirement to do some "header maintenance": Before the response is sent to client, I want all headers created by several addHeaders in a bunch of servlets, filters and other handlers to be cleaned up. In my case, this is deduplication and merging of "vary" and "cache-control" headers to assist NGINX in front so it only gets one header and not multiples.

In Jetty 9.4, I had the following code, added as bean to the connector:

  private static HttpField mergeHttpFields(String name, List<HttpField> fields) {
    return (fields == null) ? null : new HttpField(name, fields.stream()
        .map(HttpField::getValues)
        .flatMap(Stream::of)
        .distinct()
        .collect(Collectors.joining(", "));
  }
  
  private static final HttpChannel.Listener HEADER_MERGER = new HttpChannel.Listener() {
    @Override
    public void onResponseBegin(Request request) {
      final HttpFields headers = request.getResponse().getHttpFields();
      headers.computeField("Vary", MyClass::mergeHttpFields);
      headers.computeField("Cache-Control", MyClass::mergeHttpFields);
    }
  };

  // This is how its added to connector
  connector.addBean(HEADER_MERGER);

In 10.0 this no longer compiled, but was easy to fix by using the HttpFields.Mutable interface instead of just HttpFields(Response#getHttpFields() returns a mutable instance).

After trying this out, I noticed that it did not work like it did in 9.4 -- the headers were no longer merged! Debuggig through the code, I figured out that my header merger was actually called before the response was submitted and after it ran, the HttpFields were correctly merged. But on the wire they did not appear, the original headers were sent.

What changed? It looks like it is caused by the change to have mutable vs. immutable HttpFields since Jetty 10: Before the onResponseBegin() handler is called, the HttpChannel has already made an immutable snapshot of the header fields, so all changes to the mutable ones are lost. I think the problem is in HttpChannel#sendResponse(): This method calls Listener#onResponseBegin() too late, it should call this before _response.newResponseMetaData() which makes an immutable snapshot of the HttpFields: https://github.com/eclipse/jetty.project/blob/a35719367bf67611863f601eb9c150a5881556cc/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java#L1040-L1045

I think the problem could be fixed by shuffling the calls a bit. Maybe it would even be better to add another callback in the Listener like: onBeforeHeadersSend(HttpFields.Mutable fields).

How to reproduce?
Use the above code in Jetty 9.4: Here the Vary and Cache-Control headers are merged using computeField(), on Jetty 10, the method is called (I added log statements), but due to the call to Response#newResponseMetaData() before the callback, the modification to headers never go to the wire (only the snapshotted ones).

I think fixing this might be a longer process, but it would be good to have a statement by a developer how to fix this with current Jetty version - or do I need to go back to 9.4 for a while?

[uschindler]

FYI, I found a solution using an Interceptor to handle this:

final class ModifyHeaderInterceptor implements Interceptor {
  
  private final HttpChannel channel;
  private final Interceptor next;
  
  ModifyHeaderInterceptor(HttpChannel channel, Interceptor next) {
    this.channel = channel;
    this.next = next;
  }

  private static final Collector<CharSequence,?,String> COMMA_JOINER = Collectors.joining(", ");

  private static HttpField mergeHttpFields(String name, List<HttpField> fields) {
    return (fields == null) ? null : new HttpField(name, fields.stream()
        .map(HttpField::getValues)
        .flatMap(Stream::of)
        .distinct()
        .collect(COMMA_JOINER));
  }
  
  @Override
  public void write(ByteBuffer content, boolean last, Callback callback) {
    if (!channel.isCommitted()) {
      final HttpFields.Mutable headers = channel.getResponse().getHttpFields();
      headers.computeField("Vary", ModifyHeaderInterceptor::mergeHttpFields);
      headers.computeField("Cache-Control", ModifyHeaderInterceptor::mergeHttpFields);
    }
    next.write(content, last, callback);
  }

  @Override
  public Interceptor getNextInterceptor() {
    return next;
  }
  
}

I am installing this interceptor inside the onRequestBegin() callback of the listener.

This is a workaround but far from elegant.

[joakime]

Very detailed issue, thank you.

Looking at this today.

[joakime]

Opened PR #7850 with proposed fix

[uschindler]

I reviewed the PR: looks fine to me. I did no local tests, but the fix was what I expected and the test is basically the code from above. I was just not sure if moving the listener callback may have any other side effects, but after reviwing again the order does not matter (except for this issue).

[uschindler]

Some idea related to this issue:

Do you think it might be useful to have a ready-to-use handler/wrapper like this to allow users to "normalize" headers? To me Cache-Control and Vary headers are typical examples that should be deduplicated and merged together, if you check some web applications and look at the outputs it looks like a desaster of duplicates.

If you think this is a good idea, we could add a ready-to-use implementation in a separate feature request. At least there are multiple ways to do this.

[uschindler]

Thanks!