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

Issue #5214 - Servlet HEAD doesn't support content-length over Integer.MAX_VALUE #5215

Merged
merged 7 commits into from
Sep 2, 2020

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Aug 31, 2020

No description provided.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
+ In the case of HEAD, the servlet-api response is a wrapper
  of javax.servlet.http.HttpServlet$NoBodyResponse
  We know the content_length, use it.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added the Bug For general bugs on Jetty side label Aug 31, 2020
@joakime joakime self-assigned this Aug 31, 2020
@joakime joakime linked an issue Aug 31, 2020 that may be closed by this pull request
@joakime joakime requested a review from gregw August 31, 2020 14:45
joakime and others added 4 commits August 31, 2020 11:03
+ Adding DefaultServlet.doHead() to avoid servlet wrapping
+ Making ResourceService HEAD aware

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM, but t hen I wrote half of it.... need another reviewer :)

@gregw gregw requested a review from sbordet August 31, 2020 16:50
@ebremer
Copy link

ebremer commented Aug 31, 2020

Thank you genetlemen! I will retest after release. Any ETA on that?

@gregw
Copy link
Contributor

gregw commented Sep 1, 2020

@ebremer I believe we are doing releases in the next week or two.... but if you could test this before we release, then that helps us get to a release sooner!

@ebremer
Copy link

ebremer commented Sep 1, 2020

@gregw I am building 9.4.x now. I will let you know how it does.

@ebremer
Copy link

ebremer commented Sep 1, 2020

@gregw if the build is 9.4.32-SNAPSHOT, it's still failing.

@ebremer
Copy link

ebremer commented Sep 1, 2020

@gregw Nevermind. The PR was not merged yet. I rebuilt using jetty-9.4.x-5214-head-huge-static and it works fine now on my installation.

Request request = client.newRequest(destUri)
.method(HttpMethod.HEAD);
request.send(responseListener);
Response response = responseListener.get(5, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

responseListener.get() only waits for the headers, so to be sure here I would also do (maybe in a try-with-resources):

assertEquals(-1, response.getInputStream().read());

which would guarantee that the HEAD response really came without body, as it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
But if that fails, does that mean that both jetty server and jetty client have bugs then? ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does not matter - we will have a failed test and we'll investigate :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@joakime I'll fix this to get this merged ASAP.

 + check no content in HEAD response
@gregw gregw requested a review from sbordet September 2, 2020 11:01
@joakime joakime merged commit 5fef140 into jetty-9.4.x Sep 2, 2020
@joakime joakime deleted the jetty-9.4.x-5214-head-huge-static branch September 2, 2020 17:54
gregw added a commit that referenced this pull request Sep 8, 2020
Fixed gzip test as HEAD requests now have identical headers to GET.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
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 this pull request may close these issues.

Servlet HEAD doesn't support content-length over Integer.MAX_VALUE
4 participants