Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It's too hard to turn off Gzip selectively #1673

Closed
LeDominik opened this issue Aug 5, 2016 · 7 comments · Fixed by #1685
Closed

It's too hard to turn off Gzip selectively #1673

LeDominik opened this issue Aug 5, 2016 · 7 comments · Fixed by #1685

Comments

@LeDominik
Copy link
Contributor

I wanted to use Jersey's SSE support with Dropwizard 1.0 however just as described in JERSEY-3000 Dropwizard by default has GZip-Compression. I somehow didn't want to go the easy way (just turn off Gzip), so I tried a lot:

  • modifying response headers in JAX-RS by @Contexting all kind of ServletContexts
  • intercepting in Jersey with a Pre-Matching filter as well as a name-bound interceptor
  • servlet filters

Digging deeper I finally hit the underlying Jetty GzipHandler which happens to have the perfect excludedPath (exclude a path from Gzip treatment) and excludedMimeTypes (exclude a MIME type) methods that would not require me to re-create the positive whitelist of stuff that shall be Gzipped (supported in configuration)...

I guess this should be the result of getApplicationContext().getGzipHandler() but this one is null no matter where in the lifecycle. I ended up learning something about Jetty and came up with this (which works nicely):

//env.getApplicationContext().getGzipHandler(); // <-- this is always null? why?!?
env.lifecycle().addServerLifecycleListener(new ServerLifecycleListener() {
    @Override
    public void serverStarted(Server server) {
        Handler handler = server.getHandler();
        LOG.debug("Disable GZip for SSE: Starting with handler {}", handler);
        while (handler instanceof HandlerWrapper) {
            handler = ((HandlerWrapper) handler).getHandler();
            LOG.debug("Disable GZip for SSE: Found handler {}", handler);
            if (handler instanceof BiDiGzipHandler) {
                LOG.info("Excluding mime-type {} from gzip compression handler", SseFeature.SERVER_SENT_EVENTS);
                ((BiDiGzipHandler) handler).addExcludedMimeTypes(SseFeature.SERVER_SENT_EVENTS);
            }
        }
    }
});

I really think either MIME type text/event-stream should be excluded by default or flushing fixed somehow or at the blacklist be exposed for configuration or some special annotation to whitelist... No idea what is the most idiomatic way...

@LeDominik LeDominik changed the title Honestly -- it's too tough to disable GZip It's too hard to turn off Gzip selectively Aug 5, 2016
@evnm
Copy link
Member

evnm commented Aug 11, 2016

Hello Dominik. Thanks for the report.

I'd rather not change default behavior. I think the best and least-surgical solution would be to simply add accessor methods for excluded paths and MIME types to io.dropwizard.jetty.GzipHandlerFactory. This could feasibly be included in the 1.1.0 release of Dropwizard.

Can you clarify what you mean by "flushing fixed somehow?"

Regarding the getApplicationContext().getGzipHandler() oddity, it makes sense that it would return null, because the GzipHandler installed by Dropwizard wraps the application handler. I was just in this code the other day and noticed this disclaimer in Environment.java. I'm not sure why methods intended for internal use were left public, but perhaps it was accidental that a path to the getGzipHandler() method was left open in the first place.

@LeDominik
Copy link
Contributor Author

LeDominik commented Aug 11, 2016

Thanks! Regarding flushing -- this is my theory: in the end you have a org.glassfish.jersey.media.sse.EventOutput which implements a ChunkedOutput<OutboundEvent> on which one calls write(event) each time something should go to the browser. If you look at the source of this you'll see that it writes out the event as mandated by the SSE standard and then calls flushQueue.

I did not invest the time to find out exactly how this works but my suspicion is (because the GZipHandler has no flush()) that there's just no mechanism to pass on the command: write out your buffers even if they're not yet filled up! So I suspect my nice SSE messages are hanging around in the 8kByte GZIP buffer...which especially with short messages takes some time to fill.

Because AFAIK in general there's no reason why you couldn't compress that stream...

Regarding the exposure in the API I have two thoughts:

  • I understand that you want to have a minimal API surface for which guarantees are provided; I guess technically speaking you could switch out Jetty without breaking the API compatibility
  • But sometimes you want to dig deeper, and I think there should be something like an @Experimental API that allows one to access Jetty's handlers & all the fun... maybe mark them deprecated so every IDE screams at you: be sure you know what you're doing... otherwise it leads to workarounds -- I mean this is a ton of code depending on

@evnm
Copy link
Member

evnm commented Aug 12, 2016

Would additional GzipHandlerFactory configuration not solve your problem, though? We should focus on a near-term solution which solves your problem before considering wider refactoring work.

While GzipHandler has no flush method, it does have a method to enable/disable synchronous flushing. Perhaps this too could be made configurable in Dropwizard.

This is an interesting case, given that a preference for small, timely messages is fundamentally at odds with message compressibility. It seems to me that the Deflator.SYNC_FLUSH behavior might be the closest available option to what you need.

@LeDominik
Copy link
Contributor Author

LeDominik commented Aug 12, 2016

Nice catch @evnm -- setting setSyncFlush(true) works nicely 👍

However a very fun thing happens in Chrome (53.0.2785.57 beta (64-bit) on OS X to be precise)...

With sync flush enabled everything works (JavaScript receives events) but while the thing is running the inspector doesn't show the events:
gzip with syncflush - no live preview
(if you click on preview it shows nothing. Once the stream has ended the "EventStream" tab becomes available)

Turn the compression off and you'll get a live preview:
without compression - live preview 1
Live Preview:
without compression - live preview 2

But that's really a non-issue. Thinking about it sync-flush is probably fine for most applications because typically we just have JAX-RS resources returning a full JSON (so no chunking at all, everything comes in one "big bang" -- sync-flush makes no difference in this situation) or we're heading for streaming and want a flush() to flush() 😄

So 👍 if this becomes configurable; thanks again!

@evnm
Copy link
Member

evnm commented Aug 12, 2016

Weird. I have no idea how Chrome populates that EventStream tab, but I imagine that it's outside of Dropwizard's scope.

Shifting discussion over to your PR from here.

gwittel pushed a commit to gwittel/airpal that referenced this issue Aug 17, 2016
…fered.

Upon later dropwizard upgrade we should consider a more fine tuned
approach using 'syncFlush=true' fix from dropwizard/dropwizard#1673.
@jimm-porch
Copy link

If you wanted to just completely turn off GZIP in Dropwizard 1.x endpoints, how would you do it? I'm trying to find out how to do it, it seems like the feature disappeared with 1.x

@evnm
Copy link
Member

evnm commented Sep 13, 2016

@jimm-porch server.gzip.enabled is still present in 1.x and should continue to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants