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

End Vert.x response before propagating mapped exception in VertxBlockingoutput #21842

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

pedroigor
Copy link
Contributor

Relates to: #21841

@pedroigor pedroigor requested a review from geoand December 1, 2021 12:58
@geoand geoand requested a review from stuartwdouglas December 1, 2021 12:58
@pedroigor
Copy link
Contributor Author

@geoand Took more time than expected because I was trying to figure out how to force the error using tests. I failed, if you think a test is a must, do you have an idea how to force the behavior using a test?

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

I believe we've had a few of similar fixes which can be hard to reproduce with tests

@gsmet
Copy link
Member

gsmet commented Dec 1, 2021

Let's wait for @stuartwdouglas 's blessing before merging this, please.

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 1, 2021

Failing Jobs - Building 8bea56c

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.MultiModuleIncludedBuildTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@pedroigor
Copy link
Contributor Author

pedroigor commented Dec 1, 2021

@sberyozkin @geoand To reproduce this one we need to somehow force an error on the write stream so that the exception is set here

.

Then, the write method should be called.

We should be able to check by looking if headers were written, or if handlers registered via RoutingContext (e.g.: body, headers end handler, etc) are invoked.

@stuartwdouglas
Copy link
Member

I am ok with skipping a test for this one. We do have a couple of similar tests for the read side such as https://github.com/stuartwdouglas/quarkus/blob/04aa95308cc3296ec89871932cfae101daf9d46a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/IncompletePostTestCase.java#L38

Basically if you want to test this you need to manually send a HTTP request over a socket, then close the socket after a delay without reading, and also write a large amount of data in the test. Even then I think it will be hard to get a test that passes reliably though.

@pedroigor
Copy link
Contributor Author

@stuartwdouglas Yeah .. I noticed there are some tests there doing something similar you are suggesting, but I failed to adapt it to my use case :(. But I was not sure what I was doing was the right path, now that you suggested I'll try more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants