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

Can GzipHandler check if .gz file exists only by some paths? #987

Closed
btd opened this issue Oct 6, 2016 · 9 comments
Closed

Can GzipHandler check if .gz file exists only by some paths? #987

btd opened this issue Oct 6, 2016 · 9 comments

Comments

@btd
Copy link
Contributor

btd commented Oct 6, 2016

I think title is pretty clear, but would like to explain my usecase (i think it is pretty common). We have some asserts (js and css file) that we build using some frontend tools and we can prepare .gz files exactly for them. It makes more sense to add filter by path exactly for this files (or prefixes) to do not check each request for extract IO if such file +.gz exists.

What do you think?

@sbordet
Copy link
Contributor

sbordet commented Oct 6, 2016

Are you proposing that we match realPath against a list of strings that you would be able to configure in GzipHandler ?

So basically replacing a single File.exists() call with possible multiple String.startsWith() (or similar) ?

@joakime
Copy link
Contributor

joakime commented Oct 6, 2016

GzipHandler has configurables for include path and exclude path.

And those paths are checked before the *.gz tests.

Also the *.gz check (and associated checkGzExists boolean configurable) is only an optimization to test for the existence of gzip work that the DefaultServlet will actually perform.

@btd
Copy link
Contributor Author

btd commented Oct 6, 2016

Common include and exclude path is not working for my case.

I want all my 'allowed' dynamic resources will be gzipped as it now. But only some of them e.g by path /public/* or /assets/* will be checked if .gz file presented (because i know exactly this file i prepare).

@sbordet
Copy link
Contributor

sbordet commented Oct 6, 2016

@btd so answers to my questions are "yes" and "yes" ?

I'm not sure a single check to the filesystem is slower than multiple checks on strings.

@joakime
Copy link
Contributor

joakime commented Oct 6, 2016

I wonder if the entire check for *.gz can even be removed from GzipHandler and just have those served from DefaultServlet (like it would be without GzipHandler), but with GzipHandler being smart enough to not recompress the content served from DefaultServlet does (like setting the ETag header on the response)

@gregw what you you think?

@gregw
Copy link
Contributor

gregw commented Oct 6, 2016

@joakime I'm struggling to remember why we actually do need to check for the *.gz in the gzip handler still. It made sense when we wanted to avoid wrapping as the filter did (as this would prevent efficient sendContent calls).

But now with the interceptor, I don't see there is much need. It will install an interceptor, but it is smart enough to see the content is already compressed.

The ability to disable the check already exists, so I'd suggest some benchmarking with and without to see which is best.

@joakime
Copy link
Contributor

joakime commented Mar 16, 2018

The biggest impact on the check gz test in GzipHandler is when a servlet base resource is a collection of paths to check.
We should remove this check if its not needed anymore.

joakime added a commit that referenced this issue Mar 16, 2018
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor

joakime commented Mar 16, 2018

Pushed commit c008f90 to branch jetty-9.4.x-issue-987-gziphandler-gzexists that just removes the whole check-gz-exists concept from GzipHandler.
Local testing shows no test failures using this commit. Going to let CI do a build once too.

joakime added a commit that referenced this issue Mar 27, 2018
+ Flag existing methods / fields as deprecated
+ Indicate removal in Jetty 10.x

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Mar 27, 2018
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Mar 28, 2018
…xists

Issue #987 - remove checkGzExists from GzipHandler entirely
joakime added a commit that referenced this issue Mar 28, 2018
…-disable

Issue #987 - move checkGzExists default to false
@joakime
Copy link
Contributor

joakime commented Mar 28, 2018

Closing as fixed.

The next jetty-9.4.x release will have checkGzipExists set to false by default.

jetty-10.0.x has the entire concept removed from GzipHandler.

@joakime joakime closed this as completed Mar 28, 2018
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

4 participants