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

Use latest SmallRye Context Propagation #41064

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Use latest SmallRye Context Propagation #41064

merged 2 commits into from
Jun 11, 2024

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Jun 7, 2024

Fixes #40852

@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jun 7, 2024
@gsmet gsmet changed the title Use latest SR/CP Use latest SmallRye Context Propagation Jun 7, 2024
@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2024

I suppose this may be related…

2024-06-07T15:26:06.5371531Z [ERROR] io.quarkus.vertx.deployment.MessageConsumerFailureTest.testFailure
2024-06-07T15:26:06.5373087Z [ERROR]   Run 1: MessageConsumerFailureTest.testFailure:49->verifyFailure:110 expected: <boom> but was: <java.io.IOException: boom>
2024-06-07T15:26:06.5374337Z [ERROR]   Run 2: MessageConsumerFailureTest.testFailure:49->verifyFailure:110 expected: <boom> but was: <java.io.IOException: boom>
2024-06-07T15:26:06.5375516Z [ERROR]   Run 3: MessageConsumerFailureTest.testFailure:49->verifyFailure:110 expected: <boom> but was: <java.io.IOException: boom>
2024-06-07T15:26:06.5376704Z [ERROR]   Run 4: MessageConsumerFailureTest.testFailure:49->verifyFailure:110 expected: <boom> but was: <java.io.IOException: boom>

@geoand
Copy link
Contributor

geoand commented Jun 7, 2024

It very much does look related

@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2024

Yeah, I can reproduce locally. Pity that this behaviour is not tested in the TCK, or the SR/CP tests, or the Quarkus CP tests, and it shows up in something completely unrelated.

@geoand
Copy link
Contributor

geoand commented Jun 7, 2024

Yeah... But it could be worse: CI could have passed and some user's code suddenly broke for no apparent reason in a future release

This comment has been minimized.

@FroMage
Copy link
Member Author

FroMage commented Jun 10, 2024

I've looked at the problem, and I wonder if that test is actually correct. I would like to know the reasoning behind sometimes having the exception wrapped in CompletionException and sometimes not:

    @Test
    public void testFailure() throws InterruptedException {
        verifyFailure("foo", "java.lang.IllegalStateException: Foo is dead", false);
        verifyFailure("foo-message", "java.lang.NullPointerException", false);
        verifyFailure("foo-completion-stage", "java.lang.NullPointerException: Something is null", false);
        verifyFailure("foo-completion-stage-failure", "boom", true);
        verifyFailure("foo-uni", "java.lang.NullPointerException: Something is null", false);
        verifyFailure("foo-uni-failure", "boom", true);

        verifyFailure("foo-blocking", "java.lang.IllegalStateException: Red is dead", false);
        verifyFailure("foo-message-blocking", "java.lang.NullPointerException", false);
        verifyFailure("foo-completion-stage-blocking", "java.lang.NullPointerException: Something is null", false);
        verifyFailure("foo-completion-stage-failure-blocking", "boom", true);
        verifyFailure("foo-uni-blocking", "java.lang.NullPointerException: Something is null", false);
        verifyFailure("foo-uni-failure-blocking", "boom", true);
    }

All those with NameOfException: prefixes are (I think) wrapped. Only two are not (boom) and I'm not sure if that test is correct. Perhaps they should all be wrapped, or nobody should care if they are.

I'll need @cescoffier or perhaps @jponge to help me figure out this test.

@FroMage FroMage marked this pull request as draft June 10, 2024 10:04
@FroMage
Copy link
Member Author

FroMage commented Jun 10, 2024

I've updated the vertx test temporarily, and marked this PR as Draft, to see if we have other issues.

@cescoffier
Copy link
Member

We use CompletionException when using CompletionStage (that's part of the contract) or Uni failing with a checked exception (we have to wrap it anyway). These are only used when "awaiting" for a result (.get() for CS (via completable future) or await().... for Uni)

@cescoffier
Copy link
Member

The change in the test looks fine - we may have tested an implementation-specific behavior.

@FroMage
Copy link
Member Author

FroMage commented Jun 10, 2024

The change in the test looks fine - we may have tested an implementation-specific behavior.

You can't imagine how happy I am to hear those words 😥

@FroMage FroMage marked this pull request as ready for review June 11, 2024 09:26
@FroMage
Copy link
Member Author

FroMage commented Jun 11, 2024

OK, bumped to 2.1.2 which is compiled for Java 11, let's re-run CI but it should be fine.

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2024
@FroMage
Copy link
Member Author

FroMage commented Jun 11, 2024

CI takes +4h to run, now?

@geoand
Copy link
Contributor

geoand commented Jun 11, 2024

It all depends on how many PRs are running in parallel (and what modules they touch)

Copy link

quarkus-bot bot commented Jun 11, 2024

Status for workflow Quarkus CI

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

✅ 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 17

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

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

⚙️ JVM Tests - JDK 21

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

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/virtual-threads/grpc-virtual-threads

io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testUnary - History

  • INTERNAL: Half-closed without a request - io.grpc.StatusRuntimeException
io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request
	at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:268)
	at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:249)
	at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:167)
	at io.grpc.testing.integration.TestServiceGrpc$TestServiceBlockingStub.unaryCall(TestServiceGrpc.java:220)
	at io.quarkus.grpc.example.streaming.VirtualThreadTestBase.testUnary(VirtualThreadTestBase.java:33)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:1017)

@geoand geoand merged commit f6de420 into quarkusio:main Jun 11, 2024
65 of 67 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 11, 2024
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.2 Jun 11, 2024
@FroMage FroMage deleted the 40852 branch June 12, 2024 08:53
@gsmet gsmet modified the milestones: 3.11.2, 3.8.6 Aug 14, 2024
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.

@CacheResult with method returning Uni makes cache exceed its maximum size
5 participants