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

Fix HugeResourceTest #9411

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Fix HugeResourceTest #9411

merged 3 commits into from
Feb 27, 2023

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Feb 23, 2023

Fixes #9408.

Fixed double-release bug in MultiPartFormData and used ByteBufferPool.NonPooling pool to work around #9311.

@lorban lorban added Bug For general bugs on Jetty side Jetty 12 labels Feb 23, 2023
@lorban lorban requested a review from gregw February 23, 2023 10:54
@lorban lorban self-assigned this Feb 23, 2023
@lorban lorban linked an issue Feb 23, 2023 that may be closed by this pull request
HugeResourceTest failing #9408
Closed
@lorban lorban requested a review from joakime February 23, 2023 13:34
sbordet
sbordet requested changes Feb 23, 2023
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'm confused about the tests for 4g and 10g... They're supposed to be run?

jetty-ee10/jetty-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/HugeResourceTest.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/HugeResourceTest.java Outdated Show resolved Hide resolved
@joakime
Copy link
Contributor

joakime commented Feb 23, 2023 •
edited
Loading

I'm confused about the tests for 4g and 10g... They're supposed to be run?

IMO, the 4g test should (at a minimum) be enabled.
That exists due to a regression we had with FileChannel behaviors across the 2GB limit (per the JVM API).

Both were disabled less than a day ago by @gregw via commit 791c6a7

@lorban
Copy link
Contributor Author

lorban commented Feb 24, 2023

I've re-enabled 4G and 10G tests and I increased the timeout to 60s as 10G was passing right at the limit for testUploadMultipart and testUploadMultipartDelayed.

@lorban lorban requested a review from sbordet February 24, 2023 09:51
@lorban
#9408: restored HugeResourceTest:
bbbeed7 
 - fixed double-release bug in MultiPartFormData
 - used ByteBufferPool.NonPooling pool to work around #9311

Signed-off-by: Ludovic Orban <[email protected]>
@lorban
#9408: handle review comments
7e8b577 
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
#9408: re-enable 4G and 10G tests
98822f9 
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban force-pushed the fix/jetty-12-9408-re-enable-HugeResourceTest branch from 4f14996 to 98822f9 Compare February 24, 2023 10:16
@lorban lorban merged commit e819809 into jetty-12.0.x Feb 27, 2023
@lorban lorban deleted the fix/jetty-12-9408-re-enable-HugeResourceTest branch February 27, 2023 08:12
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

@gregw gregw gregw approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

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

Successfully merging this pull request may close these issues.

HugeResourceTest failing
4 participants
@lorban @joakime @gregw @sbordet