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

Fix gRPC context propagation #32164

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

alesj
Copy link
Contributor

@alesj alesj commented Mar 27, 2023

This fixes #32046 issue.

@alesj alesj requested a review from cescoffier March 27, 2023 15:47
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Mar 27, 2023
@alesj
Copy link
Contributor Author

alesj commented Mar 27, 2023

@cescoffier wait with the merge ... let me test this on blocking methods as well ...

@alesj alesj force-pushed the context_prop_deadline1 branch 3 times, most recently from 9cc82c2 to f12e7ad Compare March 27, 2023 16:35
@alesj
Copy link
Contributor Author

alesj commented Mar 27, 2023

@cescoffier OK, added a blocking test, ad it worked ootb

@quarkus-bot

This comment has been minimized.

@alesj alesj force-pushed the context_prop_deadline1 branch 2 times, most recently from 1798c97 to 8ca4c0b Compare March 27, 2023 19:56
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@alesj alesj force-pushed the context_prop_deadline1 branch 2 times, most recently from d4592bf to ce72b55 Compare March 28, 2023 09:48
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 except the logger api

@alesj alesj force-pushed the context_prop_deadline1 branch 2 times, most recently from e6a420a to cedf386 Compare March 28, 2023 10:11
@quarkus-bot

This comment has been minimized.

@alesj alesj force-pushed the context_prop_deadline1 branch 2 times, most recently from 949da3f to 1ed7cb2 Compare March 28, 2023 13:04
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@cescoffier
Copy link
Member

@alesj can you rebase, just to be sure?

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jun 27, 2023

Whats the status of this? If it's ready to be merged, please rebase once more and then add the waiting-for-ci label

@geoand geoand requested a review from cescoffier June 27, 2023 11:15
@cescoffier
Copy link
Member

No it's not. There is a race somewhere.

@geoand
Copy link
Contributor

geoand commented Jun 27, 2023

Understood, thanks

@cescoffier
Copy link
Member

@alesj do you believe we can make the race more easily reproducible?

@alesj
Copy link
Contributor Author

alesj commented Oct 15, 2023 via email

@cescoffier
Copy link
Member

@alesj I just pushed a one-line-fix. When creating a new context, you need to attach it explicitly. Thanks to the duplicated context handling, it will attach it to the duplicated context of the call.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 23, 2023

Failing Jobs - Building c2e2b7e

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 20

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/rest-client integration-tests/rest-client-reactive 

📦 integration-tests/rest-client

io.quarkus.it.rest.client.selfsigned.ExternalSelfSignedTestCase.should_accept_self_signed_certs line 18 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:288)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:314)

📦 integration-tests/rest-client-reactive

io.quarkus.it.rest.client.wronghost.ExternalWrongHostTestCase.restClientRejected line 28 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

@cescoffier cescoffier merged commit 32f44b5 into quarkusio:main Oct 23, 2023
32 of 33 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 23, 2023
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.

quarkus-grpc: gRPC Context including Deadline not propagated to gRPC server
3 participants