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

xds: ensure server interceptors are created in a sync context #11930

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergiitk
Copy link
Member

@sergiitk sergiitk commented Mar 1, 2025

XdsServerWrapper#generatePerRouteInterceptors was always intended to be executed within a sync context. This PR ensures that by calling syncContext.throwIfNotInThisSynchronizationContext().

This change is needed for upcoming xDS filter state retention because the new tests in XdsServerWrapperTest flake with this NPE:

Cannot invoke "io.grpc.xds.client.XdsClient$ResourceWatcher.onChanged(io.grpc.xds.client.XdsClient$ResourceUpdate)" because "this.ldsWatcher" is null

`XdsServerWrapper#generatePerRouteInterceptors` was always intended
to be executed within a sync context. This PR ensures that by calling
`syncContext.throwIfNotInThisSynchronizationContext()`.

This change is needed for upcoming xDS filter state retention because
the new tests in XdsServerWrapperTest flake with this NPE:

> `Cannot invoke "io.grpc.xds.client.XdsClient$ResourceWatcher.onChanged(io.grpc.xds.client.XdsClient$ResourceUpdate)" because "this.ldsWatcher" is null`
@sergiitk sergiitk requested a review from ejona86 March 1, 2025 02:15
@sergiitk
Copy link
Member Author

sergiitk commented Mar 1, 2025

Sidenote. I've provided FakeXdsClient#awaitRds and FakeXdsClient#setExpectedRdsCount purely for the reason of not having to explain how to await for RDS in the comment to execute.

But when I was replacing xdsClient.rdsCount.await(5, TimeUnit.SECONDS); with xdsClient.awaitRds() in all tests, I found the reason the tests got stuck running forever the other day:

Oh, Java, and its timeout-less waits.

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.

1 participant