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.

Sign up for GitHub

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

Jump to bottom

Issue #7683 Use direct buffers for gzip input/output, if configured. #7764

Closed
wants to merge 4 commits into from
Closed

Issue #7683 Use direct buffers for gzip input/output, if configured. #7764

wants to merge 4 commits into from

Conversation

jaroslawr
Copy link
Contributor

@jaroslawr jaroslawr commented Mar 21, 2022 •
edited
Loading

Fix for #7683.

jaroslawr
jaroslawr commented Mar 21, 2022
View reviewed changes
jetty-http/src/main/java/org/eclipse/jetty/http/GZIPContentDecoder.java
return;
_inflater.setInput(compressed);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I moved this up because otherwise there is a call done to _inflater.inflate with no input set yet)

@joakime
Copy link
Contributor

joakime commented Mar 21, 2022

@jaroslawr this PR has test failures in both Java 11 and Java 17.

java.util.zip.ZipException: Corrupt GZIP trailer
	at java.base/java.util.zip.GZIPInputStream.readTrailer(GZIPInputStream.java:226)
	at java.base/java.util.zip.GZIPInputStream.read(GZIPInputStream.java:120)
	at [email protected]/org.eclipse.jetty.util.IO.copy(IO.java:166)
	at [email protected]/org.eclipse.jetty.util.IO.copy(IO.java:116)
	at [email protected]/org.eclipse.jetty.servlet.GzipHandlerTest.testAsyncLargeResponse(GzipHandlerTest.java:446)

@jaroslawr jaroslawr force-pushed the jetty-10.0.x-7683-gzip-content-decoder-direct-bufs branch from ee7077c to 0b290c0 Compare March 21, 2022 19:13
@jaroslawr jaroslawr force-pushed the jetty-10.0.x-7683-gzip-content-decoder-direct-bufs branch 2 times, most recently from ab756b9 to dc6fba1 Compare March 22, 2022 14:05
lachlan-roberts
lachlan-roberts requested changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this change.

But I think we should also hear from @sbordet before merging.

@jaroslawr jaroslawr force-pushed the jetty-10.0.x-7683-gzip-content-decoder-direct-bufs branch from 9c2ba7e to 0bd1fd1 Compare March 23, 2022 14:01
@sbordet sbordet linked an issue Mar 23, 2022 that may be closed by this pull request
GZIPContentDecoder ignores setUseInputDirectByteBuffers setting and always uses non-direct buffers (causing GC locking) #7683
Closed
@joakime
Copy link
Contributor

joakime commented Mar 23, 2022

The builds are taking much much longer with this latest change.
Old builds: 1 hour 23 minutes (on average).
New builds on this PR: 2 hours (and build is canceled due to overall build timeout)

@jaroslawr
Copy link
Contributor Author

@joakime Builds on other PRs/branches are now also timing out:
https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/PR-7682/8/pipeline
https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/jetty-10.0.x/1404/pipeline
So I suppose it's some issue with the CI environment and not with the change itself?

@joakime
Copy link
Contributor

joakime commented Mar 23, 2022

@jaroslawr I see that now too. We are investigating.

@sbordet sbordet closed this Mar 24, 2022
@joakime
Copy link
Contributor

joakime commented Mar 24, 2022

Merged (manually) into jetty-10.0.x (and them merged up to jetty-11.0.x)

See: commit a357193

@joakime joakime mentioned this pull request Oct 6, 2022
Closed
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lachlan-roberts lachlan-roberts lachlan-roberts requested changes

@sbordet sbordet Awaiting requested review from sbordet

Assignees

@lachlan-roberts lachlan-roberts

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jaroslawr @joakime @lachlan-roberts @sbordet