Skip to content

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 (PR #11936) 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.

@sergiitk
Copy link
Member Author

sergiitk commented Mar 3, 2025

For posterity. Windows job had XdsClientFallbackTest failing, probably not related.

Type with non-zero subscribers is: `type.googleapis.com/envoy.config.cluster.v3.Cluster`
expected: 0
but was : 1
	at app//io.grpc.xds.XdsClientFallbackTest.verifyNoSubscribers(XdsClientFallbackTest.java:337)
	at app//io.grpc.xds.XdsClientFallbackTest.mainDown_fallbackUp_restart_main(XdsClientFallbackTest.java:322)

https://source.cloud.google.com/results/invocations/237fded9-83ba-4041-8c76-0b14bdf3787d

@sergiitk sergiitk requested a review from ejona86 March 3, 2025 18:36
@sergiitk sergiitk merged commit 1a2285b into grpc:master Mar 3, 2025
16 checks passed
@sergiitk sergiitk deleted the xds-server-tests-sync-context-v2 branch March 3, 2025 22:28
shivaspeaks pushed a commit to shivaspeaks/grpc-java that referenced this pull request May 24, 2025
…1930)

`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`
shivaspeaks added a commit that referenced this pull request May 26, 2025
#12102)

`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`

Co-authored-by: Sergii Tkachenko <[email protected]>
shivaspeaks added a commit that referenced this pull request Jun 2, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants