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

Replace jzlib with maintained library #265

Open
timja opened this issue Aug 16, 2021 · 4 comments
Open

Replace jzlib with maintained library #265

timja opened this issue Aug 16, 2021 · 4 comments

Comments

@timja
Copy link
Member

timja commented Aug 16, 2021

jzlib library seems to have been abandoned, so we should not compound the problem by using a fork.

IIRC @jtnord suggested dispensing with the org.kohsuke.stapler.compression package. I see it used in e.g. https://github.com/jenkinsci/jenkins/blob/bd7c823c2781d9282eab71a5e2967d386c629344/core/src/main/java/hudson/model/Api.java#L194-L195 and https://github.com/jenkinsci/jenkins/blob/f6378676b11288cc2525852b0a1a441d147e31d3/core/src/main/java/hudson/model/LargeText.java#L214-L219 but I think it is also used for general-purpose page rendering? We could use https://github.com/eclipse/jetty.project/tree/jetty-9.4.x/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip instead. Or we could switch to another gzip library, or another encoding altogether. https://en.wikipedia.org/wiki/HTTP_compression#Content-Encoding_tokens suggests that we could use deflate with Java Platform calls alone, or Brotli with whatever library. Or we could just remove compression under the assumption that most clients are collocated with the Jenkins service on a fast network.

Originally posted by @jglick in #263 (comment)

@jtnord
Copy link
Member

jtnord commented Aug 16, 2021

I don't think compression belongs in stapler or an app framework at all, if anything it is the job of the container. (Tomcat specifies this on the connector and jetty uses a handler)

I would suggest we remove it, and enable compression in winstone (example. (Any other users can configure their app server)

Less code FTW 🚀

@jglick
Copy link
Member

jglick commented Aug 16, 2021

remove it, and enable compression in winstone

Certainly sounds attractive in principle. It is longstanding tech debt. Open questions:

@jtnord
Copy link
Member

jtnord commented Aug 17, 2021

Does the transparent compression in Jetty also work when there are error pages, as the Stapler package attempts to deal with (IIUC)?

Can we deprecate all the classes in it and make them no-ops
Can we just deprecate..

Likely as you suggest , possibly we want to introduce a superclass to the compression filter that is a "ExceptionHandlerFilter" to catch errors so we can write diagnostic files, and then switch usage over to that from the ErrorHandler (again at our leisure)

At any case I do not think there is a too many stars we have to align if we wanted to bite the bullet.

If there is a more "modern" way to have an error handler for a webapp, possibly - we just need to have an <error-page> and use a servlet that can then pull information out of the Throwable and store it locally, as well as publish a (nicer?) page.

@jglick
Copy link
Member

jglick commented Aug 17, 2021

possibly we want to introduce a superclass to the compression filter that is a "ExceptionHandlerFilter"

My hope is that GzipHandler operates at a more basic level than filters, so we do not need to deal with interactions between compression and errors—the source of a lot of complexity in the current code.

If there is a more "modern" way to have an error handler for a webapp

Likely there is.

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

No branches or pull requests

3 participants