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

Refine how Request / Channel / Stream completion works #9684

Merged
merged 34 commits into from
May 15, 2023
Merged

Refine how Request / Channel / Stream completion works #9684

merged 34 commits into from
May 15, 2023

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Apr 27, 2023 •
edited
Loading

Refine how requests get completed.

  • Handling success / failures
  • Writing success / failures / last-write
  • Callback success / failures
  • Error handling cases
  • Bad Content-Length during error handling
  • Introduce new tests to cover the various combinations that can impact request completion.

@joakime
Restore LargeHeaderTest Test
406f74f 
+ Fix Bad Content-Length produced if write + error occurs
+ Fix race between callback failure and error handling failure
+ Introduce new ResponseCompleteTest to attempt to capture complete race issue
@joakime joakime added Bug For general bugs on Jetty side Test Jetty 12 labels Apr 27, 2023
@joakime joakime requested review from gregw and sbordet April 27, 2023 18:23
@joakime joakime self-assigned this Apr 27, 2023
joakime and others added 23 commits April 28, 2023 13:35
@joakime
Correct HttpChannelState to satisfy HttpChannelTest failures
1939ee5
@joakime
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
194e85f 
…rge-header-test
@joakime
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
13f846f 
…rge-header-test
@joakime
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
969d772 
…rge-header-test
@joakime
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
2f16af7 
…rge-header-test
@joakime
More DEBUG + ignoring failure for completeStream
287e351
@joakime
LargeHeaderTest WIP
e715dab
@joakime
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
7b52b1e 
…rge-header-test
@joakime
LargeHeaderTest WIP
d61bbf3
@joakime
LargeHeaderTest WIP
b569fad
@gregw
Added some javadoc, logging and better toString implementations to he…
42146ed 
…lp debug this issue.
@gregw
Simplified onFailure
c3dd607
@gregw
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/la…
7c721d5 
…rge-header-test
@gregw
WIP
353133f
@gregw
WIP
9d8acf3
@gregw
Getting better....
cef9455
@gregw
Getting better....
702b8f1
@gregw
Getting better....
36e5aec
@gregw
Hmmmmm
d433b08
@gregw
Working
31f9208
@gregw
Removed ErrorResponse in favour or an errorMode
f323e97
@gregw
made flaky test always fail
e48c8bc
@gregw
made flaky test always fail more debugging
d4b3082
@joakime joakime changed the title Restore LargeHeaderTest Test and address bugs with Content-Length calc and request complete race Refine Request / Handling / Callback / Failure completion May 10, 2023
@joakime joakime changed the title Refine Request / Handling / Callback / Failure completion Refine how Request / Channel / Stream completion works May 10, 2023
@joakime joakime removed the Test label May 10, 2023
@gregw
Copy link
Contributor

gregw commented May 12, 2023

@sbordet @joakime this is now green, so I think we should consider merging soon so it does not go stale. There are still somethings that need looking at, but it is better than what is there now.

gregw
gregw approved these changes May 13, 2023
View reviewed changes
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.

I'm approving, but it is mostly my own work. Got a thumbs up from @joakime, so that is an OK from him (his PR, so can't approve). @sbordet OK for a merge?

sbordet
sbordet requested changes May 13, 2023
View reviewed changes
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java Show resolved Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/CustomRequestLogTest.java Show resolved Hide resolved
@gregw gregw requested a review from sbordet May 14, 2023 09:05
sbordet
sbordet requested changes May 14, 2023
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd remove the shutdownOutput() call from SendCallback.onCompleteSuccess() and see how it goes -- that's the last nit from me.

@gregw gregw merged commit 2fdcae4 into jetty-12.0.x May 15, 2023
@gregw gregw deleted the fix/12.0.x/large-header-test branch May 15, 2023 06:15
@joakime joakime mentioned this pull request Jun 6, 2023
Closed
sbordet added a commit that referenced this pull request Jun 6, 2023
@sbordet
Fixes #8033 - Jetty-12 already completed exception in http2.tests.Max…
4e6575b 
…ConcurrentStreamsTest.

Fixed by the work done in #9684.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet mentioned this pull request Jun 6, 2023
Closed
@joakime joakime mentioned this pull request Aug 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw approved these changes

@sbordet sbordet sbordet approved these changes

Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joakime @gregw @sbordet