GZipHandler does not play nice with other handlers in HandlerCollection

[JensGabe]

Jetty version(s)
Jetty 11.0.2

Java version/vendor (use: java -version)
Temurin 17.02+8

OS type/version
Windows Server 2016

Description
Handler configuration:

HandlerCollection
 - ResourceHandler (root static files - eg contextpath = / )
 - GZipHandler
   - ContextHandlerCollection
     - ServletContextHandler
     - ContextHander (other static files - eg contextpath= /foo )
       - ResourceHandler
 - DefaultHandler

The GZIPHandler is configured with minSize=1024 and includes javascript and other text files.

When requesting a root static file with a size over 96K, the GZIPHandler will gzip the part of the content which exceeds 96K - the first 96K of the content is plain text, and the remainder looks GZipped.
(The repsonse content length matched the actual file length, and the browser (chrome) report error content-length mismatch)

If the root static file is a pre-zipped file, it works fine, also for size > 96K.
If the same static file is requested via the /foo resource handler, it works fine.
On my Developer setup (Windows 10, same Java) it works fine - I cannot get it to fail as on the Server installation.

Checking the source code of the various handlers, I noticed that all the handlers i used, except GZipHandler starts with:

if (request.isHandled()) return;

Reconfiguring handler configuration to use HandlerList instead or move the ResourceHandler into a wrapped ContextHandler in the ContextHandlerCollection will both fix the issue for me.

It seems like the GZipHandler or a buffer handling deeper in the output handling is not working as intended if it is called after another handler handles the request.

How to reproduce?
See description.

[joakime]

What is the declared context path of your ServletContextHandler ?

[JensGabe]

"/" - The request in question never touches the ServletContextHandler.

[joakime]

If your ServletContextHandler is on context path of /, then simplify your entire structure by using Servlet url-patterns properly and eliminating your ResourceHandler entries.

HandlerCollection
  - GZipHandler
    - ServletContextHandler (contextpath = /foo)
      - DefaultServlet (the default one on url-pattern `/`, using resourceBase from ServletContextHandler, serving static content from root)
      - DefaultServlet (a new one, on url-pattern `/foo/*`, using it's own resourceBase pointing to a different directory on disk)
 - DefaultHandler

See example at
https://github.com/jetty-project/embedded-jetty-cookbook/blob/jetty-11.0.x/src/main/java/org/eclipse/jetty/cookbook/DefaultServletMultipleBases.java

Why do this?

GZipHandler is not meant to be put after any handler that can potentially produce a response.
(It's fine to have it after handlers that don't ever possibly produce a response body, like RedirectHandler, SecureContextHandler, etc)

Also, mixing ResourceHandler and ServletContextHandler is not a great idea.
ServletContextHandler is terminal, once your request enters it, the context must respond (even with a 404).
There's no "optional" handling of a request in ServletContextHandler.
This means your context on /foo will likely never be entered, or if you have content on /foo in your ServletContextHandler those would never be served. (one or the other)

The most common reason people attempt to set up ResourceHandler and ServletContextHandler combined, is because they have a Servlet (usually a REST lib) set to answer on root context. This is an anti-pattern, mixing static content and REST on the same root url. If you move your REST support url to a different host (eg: https://api.company.com/, or a different url https://company.com/api/ then your efforts get trivial, and you can apply different rules for REST and static that don't overlap so much.

[JensGabe]

Thank you, and yes, I am aware of the fact that the construction is weird. (Historical reason).
The solution you described, is also what I already implemented as I stated in the bottom of the bug report.
The reason I did the bug report, was to emphasize that the GZipHandler did not function as the other handlers, and perhaps this should either be described in the documentation or refactored to always work or always fail. This very weird partial/intermident functionality is hard to debug.

[joakime]

When you see if (request.isHandled()) return; in a Handler, that's for when you are not using a handler collection (HandlerList, HandlerCollection, or ContextHandlerCollection).
It could only be triggered if you are using a HandlerWrapper that doesn't check for request.isHandled() before triggering it's wrapped handler (which would be a bug in that HandlerWrapper)

If you are using a handler collection, the request.isHandled() check is done at the collection level.

Examples:

https://github.com/eclipse/jetty.project/blob/dd2c39ecfebab149584b1fc50c513bc37717bc3d/jetty-server/src/main/java/org/eclipse/jetty/server/handler/HandlerList.java#L45-L55

https://github.com/eclipse/jetty.project/blob/dd2c39ecfebab149584b1fc50c513bc37717bc3d/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandlerCollection.java#L190-L195

https://github.com/eclipse/jetty.project/blob/dd2c39ecfebab149584b1fc50c513bc37717bc3d/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandlerCollection.java#L203-L208

[joakime]

In your setup, if the initial ResourceHandler (on /) sets request.setHandled(true), then the subsequent entry GzipHandler wouldn't even be called, as the HandlerList is the one doing the request.isHandled() check.

[JensGabe]

Thank you again for the followup, and yes I'm aware of all the points you are making - we have already restructured our handler configuration .
Our weird handler configuration has been in use and working fine for >5 year, the last ~1 year with v11 on multiple installations, until not working anymore in this one installation.
Can you shed some light on why this is the case? (Specifically as to why the GZipHandler suddenly partially gzip's files after the 96K mark)

[gregw]

@JensGabe I agree that there is a bug there. Regardless of if your setup is correct/best or not, I don't think GzipHandler should act in the way you are describing. If the request has been handled by a prior ResourceHandler, then the GzipHandler should be a noop.

[joakime]

I mocked this layout up, and the HandlerCollection (top level) does not call the GzipHandler if the ResourceHandler before it handled the request.
No code in GzipHandler is even called, the rest of the HandlerCollection (top level) is skipped.

[JensGabe]

I simplified out initial weird configuration to this oneliner - GZipHandler#handle is called on every request:

{{JETTYSERVER}}.setHandler(new HandlerCollection(new ResourceHandler(), new GzipHandler(), new DefaultHandler()));

[joakime]

A more reliable setup, use HandlerList instead of HandlerCollection

HandlerList <-- instead of HandlerCollection
 - ResourceHandler (root static files - eg contextpath = / )
 - GZipHandler
   - ContextHandlerCollection
     - ServletContextHandler
     - ContextHander (other static files - eg contextpath= /foo )
       - ResourceHandler
 - DefaultHandler

[joakime]

Opened PR #7872 (for jetty-9.4.x)