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 [email protected]

@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 <[email protected]>
@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 gregw requested review from sbordet and joakime November 11, 2020 16:46
@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
Copy link
Contributor Author

gregw commented Nov 14, 2020

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

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().

gregw added 2 commits November 16, 2020 13:10
@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 added 2 commits November 17, 2020 10:10
@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.

@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
@gregw gregw merged commit 14f94f7 into jetty-9.4.x 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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