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.

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

Ensure request body is consumed so that multipart requests with large payloads never hang when exception happens before body is consumed #44959

Conversation

michalvavrik
Copy link
Member

@michalvavrik
Copy link
Member Author

Quickstarts compilation is expected to fail until quarkusio/quarkus-quickstarts#1470 is merged.

This comment has been minimized.

@michalvavrik
Copy link
Member Author

closing as I think @geoand should provide better solution when the time is right and discussion about this finished

@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Dec 6, 2024
@michalvavrik michalvavrik deleted the feature/consume-body-for-exceptional-multipart-req branch December 6, 2024 10:52
@michalvavrik michalvavrik restored the feature/consume-body-for-exceptional-multipart-req branch December 6, 2024 10:52
@geoand
Copy link
Contributor

geoand commented Dec 6, 2024

This absolutely fixes the issue at hand, but I am worried about out profusion of endHandlers, so I would like to see if this can be addressed centrally.

@geoand
Copy link
Contributor

geoand commented Dec 6, 2024

@michalvavrik I propose reopening the PR, because at the very least, I will reuse your tests :)

@michalvavrik
Copy link
Member Author

@michalvavrik I propose reopening the PR, because at the very least, I will reuse your tests :)

Yeah, I thought you might use these tests :-) Thanks

@michalvavrik michalvavrik deleted the feature/consume-body-for-exceptional-multipart-req branch December 6, 2024 14:31
@michalvavrik michalvavrik restored the feature/consume-body-for-exceptional-multipart-req branch December 6, 2024 14:31
@michalvavrik michalvavrik reopened this Dec 6, 2024
@geoand
Copy link
Contributor

geoand commented Dec 6, 2024

Thank you!

@quarkus-bot quarkus-bot bot removed the triage/invalid This doesn't seem right label Dec 6, 2024
@geoand
Copy link
Contributor

geoand commented Dec 6, 2024

Can you also make it a draft?

Thanks

@michalvavrik michalvavrik marked this pull request as draft December 6, 2024 14:33
@siewp
Copy link

siewp commented Dec 19, 2024

@geoand any estimate when you could provide the better solution? We need that fix actually.
Thank you!

@geoand
Copy link
Contributor

geoand commented Dec 19, 2024

Not at all unfortunately.

@geoand
Copy link
Contributor

geoand commented Dec 19, 2024

Just to be clear, the fix will be the same. What we need is way to properly manage these end handlers that are so far are popping up all over the place (and that override each other in a random order)

@michalvavrik michalvavrik marked this pull request as ready for review January 5, 2025 13:41
@michalvavrik michalvavrik force-pushed the feature/consume-body-for-exceptional-multipart-req branch from f88cdf9 to a6f9ff8 Compare January 5, 2025 13:41
@michalvavrik michalvavrik removed the request for review from geoand January 5, 2025 13:43
Copy link

quarkus-bot bot commented Jan 5, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a6f9ff8.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.traces.OpenTelemetrySpanSecurityEventsTest.testSecurityEventTypes - History

  • event executor terminated - java.util.concurrent.RejectedExecutionException
java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:353)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:346)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-quickstart

io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled(OpenTelemetryDisabledTest.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

@michalvavrik
Copy link
Member Author

I think OpenTelemetrySpanSecurityEventsTest.testSecurityEventTypes flakiness is unrelated because I can also see it was flaky in here #45322. Hmm, I didn't know about that, I am author of that test :-/

@michalvavrik
Copy link
Member Author

I have pushed updated version. @cescoffier needs to review this when the time is right for him. I'll also send email because we kinda discussed it in 2 channels in parallel :-).

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@cescoffier cescoffier merged commit 93a769b into quarkusio:main Jan 6, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Jan 6, 2025
@cescoffier
Copy link
Member

I think we should backport it to 3.15. @geoand @michalvavrik WDYT?

@michalvavrik
Copy link
Member Author

michalvavrik commented Jan 6, 2025

I think we should backport it to 3.15. @geoand @michalvavrik WDYT?

+1, I forgot to add backport labell. Adding it now. Thanks.

@michalvavrik michalvavrik deleted the feature/consume-body-for-exceptional-multipart-req branch January 6, 2025 08:48
@geoand
Copy link
Contributor

geoand commented Jan 6, 2025

I think we should backport it to 3.15. @geoand @michalvavrik WDYT?

👍

@gsmet gsmet modified the milestones: 3.18 - main, 3.17.6 Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in secured post-multipart interface with large files
5 participants