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

Undisable ee9 BlockingTest and fix HttpChannel.produceContent #12529

Merged
merged 13 commits into from
Dec 18, 2024
Merged

Undisable ee9 BlockingTest and fix HttpChannel.produceContent #12529

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e510b69
Undisable ee9 BlockingTest and fix HttpChannel.produceContent
janbartel Nov 13, 2024
222a272
Handle CoreContextRequest being null in produceContent/needContent
lorban Nov 25, 2024
546cdd9
make tests non-flaky
lorban Dec 9, 2024
debb110
fix BlockingTest.testBlockingReadAndBlockingWriteGzipped hanging b/c …
lorban Dec 9, 2024
fe418fd
handle review comments
lorban Dec 10, 2024
1a8c6f2
handle review comments
lorban Dec 10, 2024
8e33d28
handle review comments
lorban Dec 10, 2024
f90f141
add temporary debugging stack trace
lorban Dec 10, 2024
673391b
fix exception type
lorban Dec 10, 2024
c86a935
fix exception type
lorban Dec 11, 2024
bc191b5
remove leftover
lorban Dec 11, 2024
4a149e9
do not recycle HttpStreamOverHttp1 when failed so that ChannelRequest…
lorban Dec 11, 2024
9cfe2de
restore breaking the link between channel and stream
lorban Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,10 @@ public boolean needContent()
*/
public HttpInput.Content produceContent()
{
Content.Chunk chunk = getCoreRequest().read();
ContextHandler.CoreContextRequest coreContextRequest = getCoreRequest();
if (coreContextRequest == null)
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

@lorban is this reasonable?

Copy link
Contributor

@lorban lorban Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

coreContextRequest can only be null after recycle(), so returning null in this case is wrong as that implies a call to needContent() should follow.

I think returning an new ErrorContent(new IllegalStateException("Illegal read from complete request")) would be the right thing to do when coreContextRequest is null.

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 don't see the difference between returning null here, and returning null at line 179 when there is no chunk?

Copy link
Contributor

@lorban lorban Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

produceContent() returning null has a special meaning: that means there's no content available right away, but more will follow, i.e.: everything is normal, there's just no content to be read right away. Error cases OTOH should always be returned in the form of an ErrorContent object.

When coreContextRequest is null, this means the channel has been recycled, which is an error case.

By the way, this coreContextRequest null check should also be done in needContent() as both needContent() and produceContent() fulfill the same contract about the availability of content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lorban it's not possible to return ErrorContent for needContent(): you can return false or throw an Exception, which would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

@janbartel as part of my commit, I changed needContent() to make it return true when coreContextRequest is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lorban are some of these tests racy? When I run BlockingTest in ee9 and ee88 locally on my machine, they all pass. When they run in CI, ee88 all fail with an IllegalStateException due to the channel being recycled. See https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/PR-12529/13/tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@janbartel Most of those tests were flaky, so I fixed them. But testBlockingReadAndBlockingWriteGzipped() was failing on a genuine bug in HttpChannelState.ChannelResponse which I fixed too.

But I would appreciate more pairs of eyes (@gregw? @sbordet?) to look at my fix as it's touching delicate code.

Content.Chunk chunk = coreContextRequest.read();
if (chunk == null)
return null;

Expand Down
2 changes: 0 additions & 2 deletions jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/BlockingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import org.eclipse.jetty.server.handler.gzip.GzipHandler;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
Expand All @@ -51,7 +50,6 @@
import static org.hamcrest.core.Is.is;
import static org.junit.jupiter.api.Assertions.assertTrue;

@Disabled // TODO
public class BlockingTest
{
private Server server;
Expand Down
Loading