jenkinsci / stapler

Stapler web framework
BSD 2-Clause "Simplified" License
161 stars 103 forks source link

Replace jzlib with maintained library #265

Closed timja closed 4 months ago

timja commented 3 years ago

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 https://github.com/stapler/stapler/issues/263#issuecomment-899771975

jtnord commented 3 years ago

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 commented 3 years ago

remove it, and enable compression in winstone

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

jtnord commented 3 years ago

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 commented 3 years ago

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.