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

EventSource does not work with GzipHandler #4317

Closed
lglowania opened this issue Nov 15, 2019 · 13 comments · Fixed by #6976
Closed

EventSource does not work with GzipHandler #4317

lglowania opened this issue Nov 15, 2019 · 13 comments · Fixed by #6976
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@lglowania
Copy link

Jetty version
9.4.22.v20191022
Java version
11
OS type/version
Ubuntu
Description
When you enable gzip-module, the EventSource-mechanism does not work. Chrome and FF simply don't receive events. Hard to find the problem. As they send 'accept-encoding: gzip', i think it should be possible to make it work with gzip? But for now it would be better to make text/event-stream default-excluded in here:

public GzipHandler()
    {
        _methods.include(HttpMethod.GET.asString());
        for (String type : MimeTypes.getKnownMimeTypes())
        {
            if ("image/svg+xml".equals(type))
                _paths.exclude("*.svgz");
            else if (type.startsWith("image/") ||
                type.startsWith("audio/") ||
                type.startsWith("video/"))
                _mimeTypes.exclude(type);
        }
        _mimeTypes.exclude("application/compress");
        _mimeTypes.exclude("application/zip");
        _mimeTypes.exclude("application/gzip");
        _mimeTypes.exclude("application/bzip2");
        _mimeTypes.exclude("application/brotli");
        _mimeTypes.exclude("application/x-xz");
        _mimeTypes.exclude("application/x-rar-compressed");
@joakime
Copy link
Contributor

joakime commented Nov 15, 2019

Interesting.

Have you captured the traffic with Wireshark to see if it's a flush issue or a decompression issue?

@lglowania
Copy link
Author

I tried that now, but my Wireshark-skills are not good enough.

@stale
Copy link

stale bot commented Nov 16, 2020

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.

@stale stale bot added the Stale For auto-closed stale issues and pull requests label Nov 16, 2020
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label Nov 16, 2020
@rymsha
Copy link

rymsha commented Jun 3, 2021

We have reproduced it with JaxRs SSE

Simplest client

$ curl -N \
     -H 'Accept: text/event-stream' \
     -H 'Accept-Encoding: gzip' \
     -H 'Connection: Keep-Alive' \
     http://localhost:8080/events

According to Wireshark client receives a reply

HTTP/1.1 200 OK
Date: Thu, 03 Jun 2021 11:12:07 GMT
...
Content-Type: text/event-stream
Vary: Accept-Encoding
Content-Encoding: gzip
Transfer-Encoding: chunked

And after that... silence.

Actually if server gets stopped, stuck event messages get delivered!

Seems like GzipHttpOutputInterceptor is not willing to send content immediately to the client hoping to get more into its buffer for efficiency, or sending it when response is completed. With event-stream response may be never completed, and messages may be too small to fill in the buffer.

@gregw
Copy link
Contributor

gregw commented Jun 4, 2021

The event stream should probably flush after each send. I'm reluctant to exclude the mimetype by default as that would preclude implementations that do flush

@rymsha
Copy link

rymsha commented Jun 4, 2021

But it actually does flush
https://github.com/eclipse/jetty.project/blob/jetty-11.0.x/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/EventSourceServlet.java#L167
And Resteasy implementation also calls ServletResponse#flushBuffer

@gregw
Copy link
Contributor

gregw commented Jun 4, 2021

Then that definitely is a bug. However I do recall some recent work on the gzip handler, so could you try upgrading jetty (your version is it and there are some security update you should have)

@gregw gregw added the Bug For general bugs on Jetty side label Jun 4, 2021
@joakime
Copy link
Contributor

joakime commented Jun 4, 2021

Issue #4835 addressed some GzipHttpOutputInterceptor flushing issues.

Issue #4461 fixed a small gzip aggregation bug as well (which could be impacting the flushing behavior).

You'll need at least Jetty 9.4.29.v20200521 to benefit from that work.

@rymsha
Copy link

rymsha commented Jun 5, 2021

Unfortunately issue is still reproduced on 9.4.40.v20210413

@lachlan-roberts
Copy link
Contributor

lachlan-roberts commented Jun 7, 2021

@lglowania @rymsha you need to configure the syncFlush setting on your GzipHandler.
The deflater will be buffering bytes to improve its compression efficiency but this won't work very well if you intend to be streaming data.

Use the GzipHandler.setSyncFlush(true) method to set the value to true and it should work as expected with EventSource. You could just change this value in the gzip.ini/start.ini file if you are running jetty standalone.

Let me know if this fixes the issue for you.

@rymsha
Copy link

rymsha commented Jun 7, 2021

Yes, it works.

One suggestion though:
GzipHandler is already aware of a few MIME types.
And it is known that text/event-stream MIME type requires events to be sent on flush. It would be nice if GzipHandler does syncFlush = true for text/event-stream always.

@rymsha
Copy link

rymsha commented Jun 22, 2021

For JaxRS setting GzipHandler.setSyncFlush(true) is an unacceptable compromise (when single servlet is shared) It hurts performance for non SSE endpoints .

@lachlan-roberts
Copy link
Contributor

So maybe we do need to change the syncFlush setting of the GzipHttpOutputInterceptor based on the MIME type of each request.

We could add another IncludeExcludeSet for the MIME types that are used to set syncFlush. But not sure how this would work with the existing boolean configuration, maybe we only check this set if GzipHandler.syncFlush == false.

lachlan-roberts added a commit that referenced this issue Oct 20, 2021
Issue #4317 - exclude text/event-stream MIME type from GzipHandler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants