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

Stub creation has no fallback #410

Closed
asarkar opened this issue Aug 18, 2020 · 13 comments · Fixed by #413
Closed

Stub creation has no fallback #410

asarkar opened this issue Aug 18, 2020 · 13 comments · Fixed by #413
Assignees
Labels
bug Something does not work as expected high priority Should be fixed/implemented ASAP
Milestone

Comments

@asarkar
Copy link

asarkar commented Aug 18, 2020

I believe this to be a blocking regression issue.

The context

Use grpc-kotlin library to make gRPC calls.

The bug

In grpc-spring-boot-starter:v2.9.0.RELEASE, GrpcClientBeanPostProcessor tries to figure out (reflectively) the appropriate static method to create a stub. If it fails, it falls back to using the constructor (which is assumed as private, but may not be, as in the case of grpc-kotlin). In grpc-spring-boot-starter:v2.10.0.RELEASE, the fallback is not present, and thus, the stub injection blows up with an error java.lang.IllegalArgumentException: Unsupported stub type. See the class hierarchy below:

class ServiceNameCoroutineStub @JvmOverloads constructor(
    channel: Channel,
    callOptions: CallOptions = DEFAULT
  ) : AbstractCoroutineStub<ServiceNameCoroutineStub>(channel, callOptions)
abstract class AbstractCoroutineStub<S: AbstractCoroutineStub<S>>(
  channel: Channel,
  callOptions: CallOptions = CallOptions.DEFAULT
): AbstractStub<S>(channel, callOptions)

The problem, that already existed in grpc-spring-boot-starter:v2.9.0.RELEASE but was hidden by the fallback, is that there's no code in GrpcClientBeanPostProcessor to check for AbstractStub subclasses. I think the fallback to using the constructor directly is ok, as seen in the example code, and should not be removed. However, the constructor can be assumed public and there's no need to try to change its visibility.

Note that this too, will fail, if grpc-kotlin makes the constructor private and adds a static method (I created grpc/grpc-kotlin#163). The futureproof way to fix this is not to check for the stub type, but to check for the existence of static methods named new*Stub, and then comparing the return types with the declared stub type.

Stacktrace and logs

Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [com.mycompany.ServiceNameGrpcKt$ServiceNameCoroutineStub]: Unsupported stub type: com.mycompany.ServiceNameGrpcKt$ServiceNameCoroutineStub -> Please report this issue.
	at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.lambda$createStub$1(GrpcClientBeanPostProcessor.java:244)
	at java.base/java.util.Optional.orElseThrow(Optional.java:408)
	at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.createStub(GrpcClientBeanPostProcessor.java:243)
	at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.valueForMember(GrpcClientBeanPostProcessor.java:218)
	at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.processInjectionPoint(GrpcClientBeanPostProcessor.java:127)
	at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.postProcessBeforeInitialization(GrpcClientBeanPostProcessor.java:83)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.applyBeanPostProcessorsBeforeInitialization(AbstractAutowireCapableBeanFactory.java:416)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1788)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:595)

Steps to Reproduce

N/A

The application's environment

Which versions do you use?

  • Spring (boot): 2.3.1.RELEASE
  • grpc-java: 1.30.2
  • grpc-spring-boot-starter: 2.9.0.RELEASE

Additional context

  • Did it ever work before? Yes.
  • Do you have a demo? No.
@asarkar asarkar added the bug Something does not work as expected label Aug 18, 2020
@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 18, 2020

Thanks for reporting this issue.
Could you please attach a minimal demo project that I could use to reproduce the issue and confirm a potential fix?

@ST-DDT ST-DDT self-assigned this Aug 18, 2020
@ST-DDT ST-DDT added the high priority Should be fixed/implemented ASAP label Aug 18, 2020
@ST-DDT ST-DDT added this to the 2.10.1 milestone Aug 18, 2020
@asarkar
Copy link
Author

asarkar commented Aug 18, 2020

Thank you for looking into this. I'll have to create a sample project, since I discovered the issue in a work project that I can't share for obvious reasons. But I'd think if you had/wrote any tests for the StubFactory introduced in 2.10.0.RELEASE, the problem would become immediately apparent when the code tries to find an appropriate method for AbstractStub subclasses.

Let me know if you still want me to create a sample project, or if the tests I suggested would be sufficient enough to proceed.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 18, 2020

As a workaround, you could add a StubFactory with either highest or lowest priority order and re-add the old fallback logic.

@asarkar
Copy link
Author

asarkar commented Aug 18, 2020

I've downgraded to 2.9.0.RELEASE for time being. Unless you see something that I must pick up from 2.10.0.RELEASE, I'd prefer to avoid patching my code, and just wait for this issue to be fixed in the next version.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 18, 2020

But I'd think if you had/wrote any tests for the StubFactory introduced in 2.10.0.RELEASE, the problem would become immediately apparent when the code tries to find an appropriate method for AbstractStub subclasses.

We are sorry, that this update broke things for you. I talked to the developers of grpc-java and I was told, that the private constructors are considered internal api and will probably be removed in a future release (for grpc-java that is). That's the reason, why we didn't include the fallback code in the new version of our code. The issue is not that we don't have tests for the new code (we actually have them), the issue is that we didn't have tests that actually used the fallback code. It has been pure chance that it actually worked for grpc-kotlin before. I guess it's time that we add official support for it.

Let me know if you still want me to create a sample project, or if the tests I suggested would be sufficient enough to proceed.

We use the example projects as a way to identify regressions, so having a kotlin project would definitely help with that.
I'm not familiar with kotlin or the kotlin generator, so it would speed up the bugfix process, if you could provide a kotlin variant of the local example.

The next release will contain a fallback that will first try your suggested search for a matching new*Stub and then try the constructor afterwards. I will try to PR the required changes within a week, but I might need your help with verifying that it actually fixes your problem, if I don't have an example project to test it with.

@asarkar
Copy link
Author

asarkar commented Aug 18, 2020

I'm happy to provide a sample project, and help you as I can, I just don't know how you'd integrate a separate Gradle project in your automated tests. That's why I'm little apprehensive in creating a throwaway project.

As for the constructor issue, I think there's a misunderstanding here. For the generated Java classes, the constructor is private, and not to be used (you can't see them without reflection magic). For the Kotlin generated class, the constructor is public, and their samples show direct invocation. I've opened a ticket for grpc-kotlin to provide matching static methods, but that's only a nice-to-have abstraction, and they may not do it.

So the issue isn't really with using Kotlin, it's the absence of AbstractStub type handling. Even if grpc-kotlin had a static method for creating AbstractStub subclasses, 2.10.0.RELEASE would still fail because it only checks for certain type of stubs. The only way to fix this for good is what I suggested above.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 18, 2020

Which actually brings me to my first question: Does the grpc-kotlin generator require or generate the grpc-java stub methods/classes?

@asarkar
Copy link
Author

asarkar commented Aug 18, 2020

Does the grpc-kotlin generator require or generate the grpc-java stub methods/classes?

Yes, they do, AbstractStub is from Java. It's a small but interesting project from Google, you may want to look into it sometime since you maintain a similar library (for Spring).

@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 18, 2020

I'm happy to provide a sample project, and help you as I can, I just don't know how you'd integrate a separate Gradle project in your automated tests. That's why I'm little apprehensive in creating a throwaway project.

It won't run as part of the core build. It will be run using testExamples.sh or something similar. You don't have to integrate it there at all, just create a kotlin variant of
examples/grpc-lib, examples/local-grpc-client and optionally examples/local-grpc-server. A zip is fine too.

@asarkar
Copy link
Author

asarkar commented Aug 18, 2020

OK, I'll create a Kotlin client project this evening.

@asarkar
Copy link
Author

asarkar commented Aug 19, 2020

I created https://github.com/yidongnan/grpc-spring-boot-starter/pull/412 that is a standalone project using Kotlin and grpc-kotlin. If you change the grpc-spring-boot-starter version from 2.10.0.RELEASE to 2.9.0.RELEASE, the test passes. I hope this helps.

ST-DDT added a commit that referenced this issue Aug 19, 2020
@ST-DDT ST-DDT mentioned this issue Aug 19, 2020
4 tasks
@osaka911
Copy link

osaka911 commented Aug 22, 2020

i have same problem with version 2.10.0. RELEASE, i used Java library.

Unsupported stub type: com.payment.grpc.PaymentServiceGrpc$PaymentServiceBlockingStub -> Please report this issue.

i downgraded to 2.9.0.RELEASE and nothing problem.

@asarkar
Copy link
Author

asarkar commented Aug 22, 2020

@osaka911 The root cause of this issue has been found, and documented above. What is the base class for your PaymentServiceGrpc$PaymentServiceBlockingStub?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as expected high priority Should be fixed/implemented ASAP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants