Rest Client Reactive: don't retrigger client response filters when they fail #19498

michalszynkiewicz · 2021-08-19T09:04:09Z

fixes #19476

quarkus-bot · 2021-08-19T11:26:21Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `9f28a73`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.InjectQuarkusAppPropertiesDevModeTest.main line 19 - 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:166)
	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:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1 line 64 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:166)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:161)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:611)
	at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1(KafkaDevServicesContinuousTestingTestCase.java:64)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.pla...

michalszynkiewicz · 2021-08-20T06:45:57Z

failures don't look related...

quarkus-bot · 2021-08-20T11:49:13Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `cefd0c3`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 16
✖	Native Tests - Misc4	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

✖ io.quarkus.resteasy.reactive.server.test.resteasy.async.filters.AsyncRequestFilterTest.testResponseFilters line 365 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <200> but was: <500>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:510)
	at io.quarkus.resteasy.reactive.server.test.resteasy.async.filters.AsyncRequestFilterTest.testResponseFilters(AsyncRequestFilterTest.java:365)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(Quark...

⚙️ Native Tests - Misc4 #

📦 integration-tests/gradle

✖ io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests line 18 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: "SUCCESS"
 but was: null
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests(CustomNativeTestSourceSetIT.java:18)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	at org.junit.jupiter.engine....

quarkus-bot · 2021-08-20T18:06:07Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `404f64d`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1 line 64 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:166)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:161)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:611)
	at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1(KafkaDevServicesContinuousTestingTestCase.java:64)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.pla...

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

✖ io.quarkus.resteasy.reactive.server.test.resteasy.async.filters.AsyncRequestFilterTest.testRequestFilters line 118 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <200> but was: <500>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:510)
	at io.quarkus.resteasy.reactive.server.test.resteasy.async.filters.AsyncRequestFilterTest.testRequestFilters(AsyncRequestFilterTest.java:118)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(Quarku...

…ey fail fixes quarkusio#19476

quarkus-bot · 2021-08-23T08:30:16Z

Failing Jobs - Building `873c895`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.JandexMultiModuleProjectDevModeTest.main line 21 - 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:166)
	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:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

✖ io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - 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:166)
	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:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

michalszynkiewicz · 2021-08-23T11:58:09Z

the current failure is unrelated.

I found some places that missed marking the abort chain as started. @stuartwdouglas do you want to take another look?

gsmet · 2021-08-23T17:07:13Z

@michalszynkiewicz so should we merge this one or wait?

geoand · 2021-08-24T09:29:34Z

...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/PreResponseFilterHandler.java

+ * This Handler is invoked before ClientResponseFilters handler. It changes the abort handler chain
+ * to a one without the ClientResponseFilterRestHandler's so that the filters are not retriggered in case of failure


I just looked at the server case and in that case (in accordance to the spec) the following happens:

If there is an exception mapper, then the response filters are executed.
If there is no exception mapper, then the response filter are not executed.

Is there are a good reason to follow a different strategy here?

I think there are entirely different things. In the server world, an exception mapper maps an exception to a response.
For it, it may make sense to trigger response filters and react to the response.

In the client, we map a response to an exception that should be thrown to the user.

Or am I missing something?

Anyhow, the reason for the change is that if response exception filters are thrown on an exception when running response exception filters, they are triggered twice.

I guess what you say makes sense, but I am not 100% sure yet.
But let's get it in and see in practice since it's obviously better than what we have

michalszynkiewicz · 2021-08-24T10:29:24Z

@michalszynkiewicz so should we merge this one or wait?

I think it's good. I had some doubts about merging because I did some changes after Stuart's review.

quarkus-bot bot added the area/rest label Aug 19, 2021

michalszynkiewicz requested a review from stuartwdouglas August 19, 2021 09:04

gsmet added triage/backport? labels Aug 19, 2021

stuartwdouglas approved these changes Aug 19, 2021

View reviewed changes

michalszynkiewicz force-pushed the rest-client-reactive-exception-mapper-once branch from 9f28a73 to cefd0c3 Compare August 20, 2021 09:38

michalszynkiewicz force-pushed the rest-client-reactive-exception-mapper-once branch from cefd0c3 to 404f64d Compare August 20, 2021 16:05

Rest Client Reactive: don't retrigger client response filters when th…

873c895

…ey fail fixes quarkusio#19476

michalszynkiewicz force-pushed the rest-client-reactive-exception-mapper-once branch from 404f64d to 873c895 Compare August 23, 2021 06:19

geoand reviewed Aug 24, 2021

View reviewed changes

michalszynkiewicz merged commit 66f63a7 into quarkusio:main Aug 24, 2021

quarkus-bot bot added this to the 2.3 - main milestone Aug 24, 2021

gsmet modified the milestones: 2.3 - main, 2.2.0.Final Aug 24, 2021

gsmet removed the triage/backport? label Aug 24, 2021

gsmet modified the milestones: 2.2.0.Final, 2.1.4.Final Aug 25, 2021

gsmet removed the triage/backport-2.1? label Aug 25, 2021

gsmet modified the milestones: 2.1.4.Final, 2.2.0.Final Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest Client Reactive: don't retrigger client response filters when they fail #19498

Rest Client Reactive: don't retrigger client response filters when they fail #19498

michalszynkiewicz commented Aug 19, 2021

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `9f28a73`

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

michalszynkiewicz commented Aug 20, 2021

quarkus-bot bot commented Aug 20, 2021 •

edited

Loading

Failing Jobs - Building `cefd0c3`

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

⚙️ Native Tests - Misc4 #

📦 integration-tests/gradle

quarkus-bot bot commented Aug 20, 2021 •

edited

Loading

Failing Jobs - Building `404f64d`

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

quarkus-bot bot commented Aug 23, 2021

michalszynkiewicz commented Aug 23, 2021

gsmet commented Aug 23, 2021

geoand Aug 24, 2021

michalszynkiewicz Aug 24, 2021

geoand Aug 24, 2021

michalszynkiewicz commented Aug 24, 2021 •

edited

Loading

		* This Handler is invoked before ClientResponseFilters handler. It changes the abort handler chain
		* to a one without the ClientResponseFilterRestHandler's so that the filters are not retriggered in case of failure

Rest Client Reactive: don't retrigger client response filters when they fail #19498

Rest Client Reactive: don't retrigger client response filters when they fail #19498

Conversation

michalszynkiewicz commented Aug 19, 2021

quarkus-bot bot commented Aug 19, 2021 • edited Loading

Failing Jobs - Building 9f28a73

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

michalszynkiewicz commented Aug 20, 2021

quarkus-bot bot commented Aug 20, 2021 • edited Loading

Failing Jobs - Building cefd0c3

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

⚙️ Native Tests - Misc4 #

📦 integration-tests/gradle

quarkus-bot bot commented Aug 20, 2021 • edited Loading

Failing Jobs - Building 404f64d

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

quarkus-bot bot commented Aug 23, 2021

Failing Jobs - Building 873c895

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

michalszynkiewicz commented Aug 23, 2021

gsmet commented Aug 23, 2021

geoand Aug 24, 2021

Choose a reason for hiding this comment

michalszynkiewicz Aug 24, 2021

Choose a reason for hiding this comment

geoand Aug 24, 2021

Choose a reason for hiding this comment

michalszynkiewicz commented Aug 24, 2021 • edited Loading

quarkus-bot bot commented Aug 19, 2021 •

edited

Loading

Failing Jobs - Building `9f28a73`

quarkus-bot bot commented Aug 20, 2021 •

edited

Loading

Failing Jobs - Building `cefd0c3`

quarkus-bot bot commented Aug 20, 2021 •

edited

Loading

Failing Jobs - Building `404f64d`

Failing Jobs - Building `873c895`

michalszynkiewicz commented Aug 24, 2021 •

edited

Loading