Skip to content

GZipHandler does not play nice with other handlers in HandlerCollection #7858

Closed
@JensGabe

Description

@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.

Activity

added
BugFor general bugs on Jetty side
on Apr 8, 2022
joakime

joakime commented on Apr 8, 2022

@joakime
Contributor

What is the declared context path of your ServletContextHandler ?

JensGabe

JensGabe commented on Apr 8, 2022

@JensGabe
Author

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

joakime

joakime commented on Apr 8, 2022

@joakime
Contributor

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

JensGabe commented on Apr 8, 2022

@JensGabe
Author

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

joakime commented on Apr 8, 2022

@joakime
Contributor

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

joakime commented on Apr 8, 2022

@joakime
Contributor

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

JensGabe commented on Apr 11, 2022

@JensGabe
Author

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

gregw commented on Apr 11, 2022

@gregw
Contributor

@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

joakime commented on Apr 11, 2022

@joakime
Contributor

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

JensGabe commented on Apr 11, 2022

@JensGabe
Author

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

joakime commented on Apr 11, 2022

@joakime
Contributor

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
added a commit that references this issue on Apr 11, 2022

Issue #7858 - GzipHandler request.isHandled support

cbce133
joakime

joakime commented on Apr 11, 2022

@joakime
Contributor

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

added a commit that references this issue on Apr 11, 2022

Issue #7858 - Better conditional logic in GzipHandler

b03f75b

5 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugFor general bugs on Jetty side

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @gregw@joakime@JensGabe

      Issue actions

        GZipHandler does not play nice with other handlers in HandlerCollection · Issue #7858 · jetty/jetty.project