Skip to content

Conversation

namelessssssssssss
Copy link
Contributor

@namelessssssssssss namelessssssssssss commented Oct 17, 2023

Due to the design of StubServiceDescriptor, the StubMethodDescriptor with same method name will override each other in StubServiceDescriptor.

It will cause following problems:

Detail:
Currently, Dubbo3TripleGenerator will gererate 2 StubMethodDescriptor for each non-streaming method, one for sync call, the other for async call, and they share same method name.

//They have same name 
    private static final StubMethodDescriptor greetMethod = new StubMethodDescriptor("greet",
    org.apache.dubbo.samples.tri.grpc.GreeterRequest.class, org.apache.dubbo.samples.tri.grpc.GreeterReply.class, serviceDescriptor, MethodDescriptor.RpcType.UNARY,
    obj -> ((Message) obj).toByteArray(), obj -> ((Message) obj).toByteArray(), org.apache.dubbo.samples.tri.grpc.GreeterRequest::parseFrom,
    org.apache.dubbo.samples.tri.grpc.GreeterReply::parseFrom);

    private static final StubMethodDescriptor greetAsyncMethod = new StubMethodDescriptor("greet",
    org.apache.dubbo.samples.tri.grpc.GreeterRequest.class, java.util.concurrent.CompletableFuture.class, serviceDescriptor, MethodDescriptor.RpcType.UNARY,
    obj -> ((Message) obj).toByteArray(), obj -> ((Message) obj).toByteArray(), org.apache.dubbo.samples.tri.grpc.GreeterRequest::parseFrom,
    org.apache.dubbo.samples.tri.grpc.GreeterReply::parseFrom);

StubMethodDescriptor will add itself to StubServiceDescriptor when initialize:

  public StubMethodDescriptor(String methodName, Class<?> requestClass, Class<?> responseClass, StubServiceDescriptor serviceDescriptor, MethodDescriptor.RpcType rpcType, Pack requestPack, Pack responsePack, UnPack requestUnpack, UnPack responseUnpack) {
      //...
        serviceDescriptor.addMethod(this);
    }

And obviously only the last one will be stored into StubServiceDescriptor:

    public void addMethod(MethodDescriptor methodDescriptor) {
        //use method name as key
        this.methods.put( methodDescriptor.getMethodName(), Collections.singletonList(methodDescriptor));
        Map<String, MethodDescriptor> descMap = (Map)this.descToMethods.computeIfAbsent(methodDescriptor.getMethodName(), (k) -> {
            return new HashMap();
        });
        descMap.put(methodDescriptor.getParamDesc(), methodDescriptor);
    }

Why StubMethodDescriptor.returnClass is correct?
Some one provded a temporary fix in StubInvocationUtil:
image
But the returnTypes for RpcInvocation are still wrong.

Brief changelog

This PR make the async version StubMethodDescriptor declared before the sync version. It can make the sync version of StubMethodDescriptor override the async one in StubServiceDescriptor, and provides the ture retrun types when RpcInvocation initialize.
Then, for the async call, StubInvocationUtil will set returnTypes[0] to CompletableFuture.

@namelessssssssssss namelessssssssssss marked this pull request as ready for review October 17, 2023 09:35
@namelessssssssssss namelessssssssssss changed the title Fix method descriptor override issue Fix stub method descriptor override issue Oct 17, 2023
@AlbumenJ
Copy link
Member

@EarthChen @icodening PTAL

@EarthChen EarthChen requested a review from icodening October 19, 2023 02:33
Copy link
Contributor

@icodening icodening left a comment

Choose a reason for hiding this comment

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

plz fix conflicts

Copy link

sonarqubecloud bot commented Nov 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

4 participants