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 #5605 unconsumed input on sendError #5637

Merged
merged 22 commits into from Nov 18, 2020
Merged

Issue #5605 unconsumed input on sendError #5637

merged 22 commits into from Nov 18, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 10, 2020

Add Connection:close if content can't be consumed during a sendError. Processed after the request has returned to the container.

Signed-off-by: Greg Wilkins gregw@webtide.com

@gregw
Issue #5605 unconsumed input on sendError
0cdc78e 
Add Connection:close if content can't be consumed during a sendError. Processed after the request has returned to the container.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw linked an issue Nov 10, 2020 that may be closed by this pull request
Blocked IO Thread not woken #5605
Closed
@gregw gregw requested review from sbordet and lorban November 10, 2020 21:37
@gregw gregw mentioned this pull request Nov 11, 2020
Closed
@gregw gregw marked this pull request as ready for review November 11, 2020 08:45
@gregw
Update from review
4b934b5 
 + Add close on all uncommitted requests when content cannot be consumed.
@gregw
Copy link
Contributor Author

gregw commented Nov 11, 2020

@sbordet I'm now trying to consumeAll during the completion of any request without a committed response (ie when we can still add the Connection: close).

@gregw
Update from review
6ea3c05 
 + fixed comment
 + space comma
@gregw
Copy link
Contributor Author

gregw commented Nov 11, 2020

@sbordet I had to update to only try to consume the input if the response is >= 200, otherwise it was trying to consume input for 101 upgrades etc.
Don't we have a HTTP/2 case where we do an upgrade, but not with a 101? Is this going to break it?

@gregw
Updated to be less adventurous
ef560df 
I do not think it was valid to always consumeAll in COMPLETE as this could break upgrades with both 101s and 200s
Instead I have reverted to having this consumeAll logic only:
 + in sendError once control has passed back to the container and we are about to generate an error page.
 + in front of all the sendRedirection that we do without calling the application first.

Extra tests also added
@gregw
Copy link
Contributor Author

gregw commented Nov 12, 2020

@sbordet @lorban I know you have approved, but can you re-review.

I've decided to go for a less general solution as I lost confidence that consumeAll was always appropriate, as it was breaking upgrade.
Instead I'm just doing in sendError and in the many places that we do a redirection ourselves without calling the application first. This is a bit uglier, but I think it is safer.

lorban
lorban approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

You've got 1 broken test (AsyncRequestReadTest.testPartialReadThenClose) but it should be easy to fix, so this LGTM if fixing the test does not require non-test code changes (which I think is the case).

gregw and others added 5 commits November 12, 2020 17:05
@gregw
Updated to be less adventurous
8c0e4fe 
reverted test
@joakime
Testcase for odd sendError(400) issue.
727b3e1 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@sbordet
Fix for odd sendError(400) issue.
59611a4 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@joakime @gregw
Testcase for odd sendError(400) issue.
7846e0f 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@gregw
Always try to consumeAll on all requests
66ea4cc
@gregw
Copy link
Contributor Author

gregw commented Nov 14, 2020

@sbordet can you check the latest commit that always calls consumeAll

@gregw gregw added this to In progress in Jetty 9.4.35 via automation Nov 15, 2020
Jetty 9.4.35 automation moved this from In progress to Review in progress Nov 16, 2020
sbordet
sbordet requested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I think the handling of redirects must be reviewed, as we don't want to duplicate all the calls to ensureContentConsumedOrConnectionClose().

jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/FormAuthModule.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java Outdated Show resolved Hide resolved
@gregw
Fixed comment from review
bdbd5d9
@gregw
Updates from review
cbaf3bf 
+ added redirect methods that consumeAll
+ ensureContentConsumedOrConnectionClose renamed to ensureConsumeAllOrNotPersistent
+ ensureConsumeAllOrNotPersistent now handles HTTP/1.0 and HTTP/1.1 differently
@gregw gregw requested review from sbordet and lorban November 16, 2020 14:04
@gregw
update from review
5daecad 
 + better javadoc
 + filter out keep-alive
 + added more tests
@gregw
update from review
578a961 
 + better javadoc
@gregw gregw requested a review from sbordet November 17, 2020 09:15
@gregw
update from review
9a5f987 
 + fixed form redirection test for http 1.0 and 1.1
sbordet
sbordet requested changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

A couple of improvements to do.

jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Response.java Outdated Show resolved Hide resolved
@gregw
update from review
bff386b 
 + HttpGenerator removes keep-alive if close present
 + Use isRedirection
@gregw gregw requested a review from sbordet November 17, 2020 14:18
Jetty 9.4.35 automation moved this from Review in progress to Reviewer approved Nov 17, 2020
@gregw gregw merged commit 14f94f7 into jetty-9.4.x Nov 18, 2020
Jetty 9.4.35 automation moved this from Reviewer approved to Done Nov 18, 2020
@gregw gregw deleted the jetty-9.4.x-5605-unconsumed-send-error branch November 18, 2020 09:40
@lorban lorban mentioned this pull request Dec 13, 2021
Closed
@joakime joakime mentioned this pull request Mar 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@joakime joakime Awaiting requested review from joakime

@lorban lorban Awaiting requested review from lorban

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Jetty 9.4.35
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Blocked IO Thread not woken
4 participants
@gregw @joakime @sbordet @lorban