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

Override gRPC Context storage #28199

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

cescoffier
Copy link
Member

The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.

@cescoffier cescoffier added kind/bug Something isn't working triage/backport-2.13 labels Sep 26, 2022
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Sep 26, 2022
@cescoffier cescoffier force-pushed the fix-grpc-context-lost branch from a2d1b06 to ad9139f Compare September 26, 2022 11:45
@quarkus-bot

This comment has been minimized.

The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
@sberyozkin
Copy link
Member

The oidc code flow test failure is unrelated - which will be disabled in #28209

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 26, 2022

Failing Jobs - Building b82b235

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17 MacOS M1
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/oidc-code-flow 

📦 integration-tests/oidc-code-flow

io.quarkus.it.keycloak.CodeFlowTest.testTokenAutoRefresh line 509 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/tenant-autorefresh?state=d03b0a81-c439-4660-a82d-c8c04d4e2e1d&session_state=d5349c09-8a9d-41f0-96ff-ae6b0a5773d3&code=fa6c18b6-1f4c-46a1-a00e-bf8da8533486.d5349c09-8a9d-41f0-96ff-ae6b0a5773d3.dbb95df1-5a3d-4d00-a21b-54f8ec1105a1
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks fine to me but I'm not familiar with this stuff so feel free to seek for another reviewer you trust more :).

@cescoffier cescoffier merged commit 1a2504a into quarkusio:main Sep 28, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 28, 2022
@cescoffier cescoffier deleted the fix-grpc-context-lost branch September 28, 2022 11:07
/**
* Override gRPC context storage to rely on duplicated context when available.
*/
public class ContextStorageOverride extends Context.Storage {
Copy link

Choose a reason for hiding this comment

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

wow that's nasty (in gRPC) , the io.grpc.override.ContextStorageOverride hardcoded class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc gRPC kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants