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

Codegen base class for services to extend #1469

Closed
ejona86 opened this Issue Feb 23, 2016 · 22 comments

Comments

Projects
None yet
@ejona86
Copy link
Member

ejona86 commented Feb 23, 2016

We should allow adding new methods to a service without breaking existing service implementations. The best way to do that seems to have service implementations extend a base class that responds with UNIMPLEMENTED for all methods. The class would also make creating fake/mock services during tests easier.

A similar problem can happen for stubs in tests. We may need something there as well.

See also grpc/grpc#5371

Edit: IOW, remove all interfaces from the generated code, since adding a method will break existing implementations of that interface.

@louiscryan

This comment has been minimized.

Copy link
Contributor

louiscryan commented May 23, 2016

Will pick this up...

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented May 24, 2016

As part of this we'll need to update the webpage docs.

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jun 7, 2016

This issue should remain open until the @ExperimentalApi annotation is removed.

@hsaliak

This comment has been minimized.

Copy link
Member

hsaliak commented Jun 10, 2016

@louiscryan would you be able to take this on or suggest someone else to pick this up?

@ejona86 ejona86 assigned dapengzhang0 and unassigned louiscryan Jun 14, 2016

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jun 17, 2016

Edit: This turned out to be impossible. See follow-up comment.

CC: @lukaszx0, @jhspaybar, @louiscryan

This came out of a discussion with @dapengzhang0, @zhangkun83, and myself.

With the changes I'm about to suggest, all service implementations would fail to compile with newer codegen. Fixing it should be as easy as replacing one word for each service implementation. But it wouldn't be possible to have a service implementation compile with old and new codegen. Clients shouldn't generally be impacted, unless doing mocking in test code.

Assuming a proto containing:

service TestService {}

The generated interfaces TestService, TestServiceBlockingClient, and TestServiceFutureClient will become abstract classes with default implementations for all methods (so they technically don't have to be abstract, but almost every user should need to extend anyway). We are also going to rename AbstractTestService to TestServiceImplBase, because it would be confusing to have AbstractTestService when TestService is also abstract and its base class.

Rejected options

We discussed renaming TestService to AbstractTestService and renaming the existing AbstractTestService to AbstractTestServiceImpl. This hierarchy and names look nice, but client code using the base classes (to ease testing) would look uglier. Also, given that these abstract classes have no strong requirement to be abstract, in the future we may just remove the abstract keyword and let users instantiate them directly. But that also means if in the future we add back interfaces there won't be as good of names for them.

We also realized that our current naming of blocking and future was probably suboptimal. Instead of naming them TestServiceBlockingStub and TestServiceBlockingClient we should have probably gone with BlockingTestServiceStub and BlockingTestServiceClient, since BlockingTestService makes sense by itself and more closely mirrors TestService (whereas TestServiceBlocking doesn't make much sense as a class name). But changing it at this point would cause a lot of pain for probably limited benefit.

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jun 17, 2016

Aaand now we realize that the stub is now needing to extend two different classes. That's not going to work. Reworking the solution...

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jun 17, 2016

CC: @lukaszx0, @jhspaybar, @louiscryan

Take 2.

Delete TestService, TestServiceBlockingClient, TestServiceFutureClient. If they were abstract classes, using them on the client-side stub would require multiple inheritance along with with AbstractStub. They offer no real value on server-side.

Rename AbstractTestService to TestServiceImplBase. Since there is now a stronger split between client/server, we think it still makes sense to rename the class.

We will not support mocking client-side. Reconfiguration (with with*) is pretty broken when mocking, and makes is very difficult to get accurate CallOptions when mocking. To get CallOptions you are much better off using a ClientInterceptor. To implement a method you are much better off implementing the server service and using in-process transport, because it prevents emulating grpc incorrectly (and gRPC does lots of validation checks). Today Mockito is able to mock the stub, even though it lacks a public constructor. So we will mark the stubs final to prevent mocking.

To improve mocking server-side, TestServiceImplBase.bindService will be made final, which means the method is still functional when mocked. That removes any real use for the static ServiceGrpc.bindService, so it will be deprecated (for later removal) or removed now.

@jhspaybar

This comment has been minimized.

Copy link
Contributor

jhspaybar commented Jun 17, 2016

I'll read this over and comment, @elandau also.

dapengzhang0 added a commit that referenced this issue Jun 30, 2016

compiler: deprecate interfaces and add ImplBase in codegen
first step to address issue #1469:

- leave and deprecate interfaces in codegen
- introduce `ServiceImplBase`,
- `AbstractService` is deprecated and extends `ServiceImplBase`
- static `bindService()` is deprecated
@johnbcoughlin

This comment has been minimized.

Copy link
Contributor

johnbcoughlin commented Jul 6, 2016

@ejona86 can you elaborate on what you mean by this?

To implement a method you are much better off implementing the server service,

I have a class under test which depends on a FooServiceBlockingStub. If I implement a FooService, how can I get an instance of the blocking stub to pass as the fake dependency? From looking at the APIs that are exposed right now, my best guess is to use the InProcess toolchain. That's an awfully heavy-weight requirement for a simple unit test.

@zhangkun83

This comment has been minimized.

Copy link
Contributor

zhangkun83 commented Jul 6, 2016

@johnbcoughlin Yes, we suggest using in-process transport for such cases. It's actually pretty light-weight. If you pass a direct executor to the channel builder, the whole set-up won't use any extra threads, and calling a method on the stub will call your service handler in the same thread.

When you mock the stub, you are actually mocking out the gRPC client library, and put faith in that your mocks would behave in consistence with the actual gRPC library, which is usually not the case. Using in-process transports allows you to safely mocking out RPC service logic and be assured that the stubs will behave the same as with real services.

We have learned it the hard way. Google's internal RPC library allows mocking client stubs, but we had to write a whole bunch of supporting utilities to help mocked stubs behave correctly. It's not just waste of efforts, but also error-prone.

@johnbcoughlin

This comment has been minimized.

Copy link
Contributor

johnbcoughlin commented Jul 6, 2016

@zhangkun83 that all makes sense, and yeah, after sketching out what the in-process transport setup looks like, it's not bad. We are still very early grpc users, to the point where we'd actually been using the interfaces most places, so I hadn't yet felt any pain from mocking out grpc (no deadlines, no extra interceptors).

@netdpb

This comment has been minimized.

Copy link
Contributor

netdpb commented Jul 12, 2016

By switching from interfaces to abstract base classes, you prevent people from using one class to implement two or more services.

Have you considered keeping the interfaces but using default methods instead?

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jul 12, 2016

@netdpb, we have to maintain JDK 6 compatibility, since not everyone is on JDK 8 yet and it will take years for Android to have default method support in enough devices to use it.

@netdpb

This comment has been minimized.

Copy link
Contributor

netdpb commented Jul 12, 2016

What about on the server side? Do you expect Android apps to implement services?

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jul 12, 2016

Android apps tend not to implement the services, except mocking during unit testing. But given the nature of the generated code, it has been hard to have separate generated artifacts for client and server (from an infrastructure perspective; not a "it's hard to teach protoc"), especially given how they have evolved over time.

Not everyone is on Java 8 yet, so we have no real ability to require Java 8 for anything at this time. Any Java 8-based APIs must be supplemental and post-1.0.

@eamonnmcmanus

This comment has been minimized.

Copy link
Contributor

eamonnmcmanus commented Jul 16, 2016

Removing the interfaces is a bit sad-making. You can't use java.lang.reflect.Proxy with abstract classes, for example. I think it would be better if there were both an interface and a class implementing it with default implementations. Code that implements the service would typically extend the latter, but code doing things like making a dummy implementation via java.lang.reflect.Proxy could use the former.

@ejona86

This comment has been minimized.

Copy link
Member

ejona86 commented Jul 16, 2016

@eamonnmcmanus, I agree it's sad. But we can't assume that grpc users will use the interfaces correctly, like was done with Stubby. And we also can't just fix user's code when they use it incorrectly. Coupled with the semantic versioning crowd, it's hard to argue for the interfaces.

It's also unfortunate that interfaces behave as they do, though. Maybe a Java 8 codegen can add them back later. I'm not too concerned with java.lang.reflect.Proxy though, since you can already implement your ownServerCallHandler.

@pgrosu

This comment has been minimized.

Copy link

pgrosu commented Jul 16, 2016

Hi Eric,

I would have more faith in the users :) Why can't the following be a possibility using interfaces - it runs as expected, prevents method overwriting and allows for extensibility:

interface InterfaceWithFixedClass { 

    static final MethodClass Fixed = new MethodClass();  

    public static class MethodClass {

        public final String sayHi( String name ) {
            return "Hi " + name + " :)";
        }
    }
} 

class ImplementingClass implements InterfaceWithFixedClass { 

    public static void main(String [ ] args) {

        System.out.println( Fixed.sayHi("Eric"));
    }
} 

dapengzhang0 added a commit that referenced this issue Jul 21, 2016

compiler: add build option to enable deprecated generated code
partially resolving #1469

The added option for java_plugin `enable_deprecated` is `true` by default in `java_plugin.cpp`, so the generated code for `TestService.java` (`compiler/build.gradle` not setting this option) has all deprecated interfaces and static bindService method.

`./build.gradle` and `examples/build.gradle` set this option explicitly to `false`, so all the other generated classes do not have deprecated code.

Will set `enable_deprecated` to `false` by default in future PR when we are ready.

ejona86 added a commit to ejona86/grpc-java that referenced this issue Jul 25, 2016

compiler: add build option to enable deprecated generated code
partially resolving grpc#1469

The added option for java_plugin `enable_deprecated` is `true` by default in `java_plugin.cpp`, so the generated code for `TestService.java` (`compiler/build.gradle` not setting this option) has all deprecated interfaces and static bindService method.

`./build.gradle` and `examples/build.gradle` set this option explicitly to `false`, so all the other generated classes do not have deprecated code.

Will set `enable_deprecated` to `false` by default in future PR when we are ready.

Cherry-pick modifications:
  Removed ExperimentalApi import from ClientCalls.java

ejona86 added a commit to ejona86/grpc-java that referenced this issue Jul 25, 2016

dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this issue Jul 26, 2016

compiler: add build option to enable deprecated generated code
partially resolving grpc#1469

The added option for java_plugin `enable_deprecated` is `true` by default in `java_plugin.cpp`, so the generated code for `TestService.java` (`compiler/build.gradle` not setting this option) has all deprecated interfaces and static bindService method.

`./build.gradle` and `examples/build.gradle` set this option explicitly to `false`, so all the other generated classes do not have deprecated code.

Will set `enable_deprecated` to `false` by default in future PR when we are ready.

dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this issue Jul 26, 2016

@email2liyang

This comment has been minimized.

Copy link

email2liyang commented Oct 19, 2016

below is the code I need to write to test a business method which rely on grpc stub after v1.0.0.

you can see this is more complex then we have the ability to mock the stub behaviour . when you have interface, it's very easy to mock the behaviour of the stub.

is it the only reason you want do not want to break user's build when user add more method in the protobuffer definition file? in practice we always extend the XXXXImplBase class, so when a new method is added in protobuffer file , the method can be added to the interface and XXXImplBase class directly.

import com.github.javafaker.Faker;

import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import io.grpc.Channel;
import io.grpc.Server;
import io.grpc.inprocess.InProcessChannelBuilder;
import io.grpc.inprocess.InProcessServerBuilder;
import io.grpc.stub.StreamObserver;
import io.practiceinsight.licensingalert.filtermanager.generated.AllUsersResponse;
import io.practiceinsight.licensingalert.filtermanager.generated.FilterManagerServiceGrpc;
import io.practiceinsight.licensingalert.filtermanager.generated.FindAllUsersRequest;

import static org.testng.Assert.assertEquals;

/**
 * @author email2liyang@gmail.com
 */
public class BusinessTest {

  private FilterManagerServiceGrpc.FilterManagerServiceBlockingStub filterManagerServiceBlockingClient;
  private Faker faker;
  private String inProcessServerName;

  @BeforeMethod
  public void setUp() throws Exception {
    faker = new Faker();
    inProcessServerName = faker.letterify("filterManagerServer-??????");
    Channel channel = InProcessChannelBuilder.forName(inProcessServerName).usePlaintext(true).build();
    filterManagerServiceBlockingClient = FilterManagerServiceGrpc.newBlockingStub(channel);
  }

  @Test
  public void test_business_loigc_method_with_grpc_stub() throws Exception {
    String userId = faker.letterify("userId-???");
    FilterManagerServiceGrpc.FilterManagerServiceImplBase
        service =
        new FilterManagerServiceGrpc.FilterManagerServiceImplBase() {
          @Override
          public void findAllUsers(FindAllUsersRequest request, StreamObserver<AllUsersResponse> responseObserver) {
            responseObserver.onNext(AllUsersResponse.newBuilder().addUserIds(userId).build());
            responseObserver.onCompleted();
          }
        };

    Server server = InProcessServerBuilder.forName(inProcessServerName).addService(service).build();
    server.start();

    //set filterManagerServiceBlockingClient into the business object and we can expect
    //filterManagerServiceBlockingClient.findAllUsers(FindAllUsersRequest.newBuilder().build())
    //will return the userId we declar in the first line of the method
    assertEquals(filterManagerServiceBlockingClient.findAllUsers(FindAllUsersRequest.newBuilder().build()).getUserIds(0),
                 userId);
    server.shutdownNow();
  }
}
@zhangkun83

This comment has been minimized.

Copy link
Contributor

zhangkun83 commented Nov 15, 2016

@email2liyang Indeed for the trivial unary blocking call use case, it needs more boilerplate to use InProcess than mocking stub. However, when it comes to streaming and/or async calls, it becomes harder to mock, and the test becomes less. Please read my earlier comment.

Also, if your product is complex enough, different parts of the client may use different flavors of stub (async, blocking or future) for the same service. In that case, injecting an InProcess channel is way easier than mocking and injecting all flavors of stubs.

@thkiesling

This comment has been minimized.

Copy link

thkiesling commented Sep 15, 2017

Stumbled upon this issue when trying to mock a grpc client using Mockito...
I would love to see Interfaces on top of the generated stubs, but I get your points.
In the end I used the following method.

final MyResponse myTestResponse = ...
final MyServiceGrpc.MyServiceBlockingStub stub = createMock(
    MyServiceGrpc::newBlockingStub,
    new MyServiceGrpc.MyServiceImplBase() {
        @Override
        public void doStuff(MyRequest request, StreamObserver<MyResponse> responseObserver) {
            responseObserver.onNext(myTestResponse);
            responseObserver.onCompleted();
        }
    });
final MyResponse myResponse = stub.doStuff(MyRequest.getDefaultInstance());
public static <T extends AbstractStub<T>> T createMock(Function<Channel, T> stubConstructor,
 BindableService service) {

        final String serviceName = service.getClass().getSimpleName() + System.identityHashCode(service);

        // service
        final ServerImpl server = InProcessServerBuilder.forName(serviceName)
                .directExecutor()
                .addService(service)
                .build();

        try {
            server.start();
        } catch (IOException e) {
            throw new RuntimeException(e);
        }

        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
            // Use stderr here since the logger may have been reset by its JVM shutdown hook.
            System.err.println("*** shutting down gRPC server " + serviceName + " since JVM is shutting down");
            server.shutdown();
            System.err.println("*** server  " + serviceName + " shut down");
        }));

        // client
        final ManagedChannel clientChannel = InProcessChannelBuilder.forName(serviceName)
                .directExecutor()
                .build();

        final T ret = stubConstructor.apply(clientChannel);

        return ret;
    }
@email2liyang

This comment has been minimized.

Copy link

email2liyang commented Sep 29, 2017

grpc provide JUnit Rule which makes the tasks easier to mock

@Rule
  public GrpcServerRule grpcServerRule = new GrpcServerRule();

you can see full example here https://github.com/email2liyang/grpc-mate/blob/master/elasticsearch-service/src/test/java/io/datanerd/es/service/EchoServiceTest.java

@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.