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

since grpc-java 1.68.1, pekko-grpc has issues with RetryingNameResolver (throwIfNotInThisSynchronizationContext) #11662

Open
pjfanning opened this issue Nov 3, 2024 · 4 comments
Assignees
Labels
bug TODO:backport PR needs to be backported. Removed after backport complete

Comments

@pjfanning
Copy link

pjfanning commented Nov 3, 2024

Similar to #10407 but has started affecting us with grpc-java 1.68.1

Error:  [11/03/2024 08:12:31.956] [default-pekko.actor.default-dispatcher-6] [org.apache.pekko.dispatch.Dispatcher] Not called from the SynchronizationContext
java.lang.IllegalStateException: Not called from the SynchronizationContext
	at com.google.common.base.Preconditions.checkState(Preconditions.java:515)
	at io.grpc.SynchronizationContext.throwIfNotInThisSynchronizationContext(SynchronizationContext.java:134)
	at io.grpc.internal.ManagedChannelImpl$NameResolverListener.onResult2(ManagedChannelImpl.java:1686)
	at io.grpc.internal.RetryingNameResolver$RetryingListener.onResult2(RetryingNameResolver.java:107)
	at io.grpc.NameResolver$Listener2.onAddresses(NameResolver.java:228)
	at org.apache.pekko.grpc.internal.PekkoDiscoveryNameResolver.$anonfun$lookup$1(PekkoDiscoveryNameResolver.scala:56)
	at org.apache.pekko.grpc.internal.PekkoDiscoveryNameResolver.$anonfun$lookup$1$adapted(PekkoDiscoveryNameResolver.scala:53)

pekko-grpc PR: apache/pekko-grpc#397

pekko-grpc is written in Scala and we are using Scala Futures when doing lookups asynchronously. grpc-java seems now to require that we use your SynchronizationContext instead.

  • Is there anyway to disable the SynchronizationContext requirement?
  • Alternatively, is it possible to get the SynchronizationContext from a NameResolver (or its Listener)?

I added an experimental change to pekko-grpc name resolution to add blocking code. This allowed me to avoid this issue but I discovered that we have some tests that still fail because io.grpc is unhappy that we are using Scala Futures. I wouldn't be delighted about the hack in our name resolver either.

@ejona86
Copy link
Member

ejona86 commented Nov 4, 2024

@kannanjgithub, the old Listener didn't require using the synchronization context. But the new stuff does. We probably messed up one of the adapting methods with onResult2 and didn't take that into account.

@pjfanning, NameResolvers can access the SynchronizationContext from NameResolver.Args.getSynchronizationContext(). Long-term (getting nearer term), we will require NameResolvers to call the Listener from the SynchronizationContext, so that is a good change to make if you are willing.

@ejona86 ejona86 added bug and removed question labels Nov 4, 2024
@pjfanning
Copy link
Author

@ejona86 Thanks for your quick response.

Apache Pekko is quite modular and pekko-grpc relies on other pekko modules including a pekko-discovery module. pekko-discovery has async APIs using Scala Futures. We won't be able to change this. The custom NameResolver in pekko-grpc uses the pekko-discovery APIs.
In the short term, we will pin to grpc-java 1.67. In the long term, we will need to watch and see how grpc-java continues to evolve and whether we need to substantially rewrite pekko-grpc to uptake newer versions of grpc-java.

kannanjgithub added a commit to kannanjgithub/grpc-java that referenced this issue Nov 5, 2024
…ization context to call it from inside of the synchronization context.

Fixes grpc#11662.
@kannanjgithub
Copy link
Contributor

kannanjgithub commented Nov 5, 2024

The problem was with the gRPC code NameResolver.Listener.onAddresses abstract base class implementation calling onResult2 outside of the synchronization context. In the first part of the changes for onResult2 it was calling onResult like it should but the later PR for ResolutionResult introduced this regression.
This (and 2 other such callers) went uncaught because the check for call from the synchronization context is in ManagedChannelImpl's implementation of Listener2 but respective unit tests of the name resolvers use their own listeners.
I have raised PR #11666 with the fix.

@kannanjgithub kannanjgithub added the TODO:backport PR needs to be backported. Removed after backport complete label Nov 5, 2024
@ejona86
Copy link
Member

ejona86 commented Nov 5, 2024

whether we need to substantially rewrite pekko-grpc to uptake newer versions of grpc-java.

@pjfanning, I don't see what substantial changes you are talking about. Somewhere between onComplete and calling the listener call syncContext.execute and run the remaining code inside. I don't know Scala, but it would be a one line change in Java.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug TODO:backport PR needs to be backported. Removed after backport complete
Projects
None yet
Development

No branches or pull requests

3 participants