L8: Separate internal-only and public sections of the C++ wrapping

[vjpai]

No description provided.

[markdroth]

https://www.youtube.com/watch?v=QqreRufrkxM

[vjpai]

Resolved.

[markdroth]

The struct internal thing seems a bit ugly to me. Can we instead make the static factories private, with appropriate friendship?

[vjpai]

The issue here is that these structs get created in functions that are created by the code generator. Those functions live in a class called Stub but that can be in literally any namespace (since the namespace is set in the proto file). This class can also have any function name (since the proto file also specifies the acceptable rpc names). So there's no clear way of specifying either the class or the function to friend.

[vjpai]

I should have clarified: struct internal is defined separately in each of the classes that is being modified. So instead of calling grpc::ClientAsyncResponseReader::Create, the code generator will call grpc::ClientAsyncResponseReader::internal::Create . It's not one big struct internal which the document might have made it look like.

[vjpai]

I think that I've resolved the comments from @markdroth but would appreciate confirmation or any other review comments.

[yang-g]

While we never had something this explicit, we did mention anything under impl/ is not public API. But we did not really have a way to enforce that anyway.

I am not sure the Interfaces should to be marked internal, a user can implement those in a test or something. Even for the reader/writer classes, since we are returning those objects to the user, is it a good idea to mark them internal? The same question applies to the ChannelInterface and ServerInterface.

[vjpai]

Addressed remaining comments. PTAL!

[yang-g]

LGTM

[vjpai]

If approvals are met, the comment period for this gRFC will close on July 11 (extended by 2 days to account for the fact that July 3 and 4 are not business days for the gRPC team)

[vjpai]

Rebased to squash commits; diff unchanged.

[vjpai]

Updated approver and approval status; squashed commits, rebased. Thanks for all who checked this out! Will merge on 7/11 unless blocked by further comments.

[ghemawat]

For a harder API boundary, consider adding an extra rule: no public header includes a non-public header. This can become an automated test and will keep things sane in the future.

The downside is that templated APIs are hard to deal with this way since C++ likes for their implementation to be moved into the .cc file.

[vjpai]

I think that the template issue has been exactly our problem in the past: we have said that include/grpc++/impl is not public but then many headers in include/grpc++ reach into that to provide the needed templates. Our current feeling (as indicated in our BUILD files, for example) is that anything under include/grpc++ is ultimately public but we can make it clear what is and isn't really meant to be used by applications by spelling out the internalization directly.

[ghemawat]

By having a separate category for "code-generation layer", you prevent external entities from writing their own code generation tools. How hard would it be remove this category.

[vjpai]

Good point; this should at the very least say "through a code-generation layer" since google/flatbuffers is an example of an external code-generator that interfaces with gRPC. The point, in this case, is that a code-generator like flatbuffers will pick up gRPC at release points and make the changes needed in their generated code at those points. Do you think that changing "the" to "a" would resolve this issue to your satisfaction?

[vjpai]

The first one should probably also say "Documented for public use by end-user applications" just to make it clear that we're talking about client and server applications in that category and not other libraries like flatbuffers.

[chwarr]

I'm a bit late to the party here, so sorry about that.

I implement codegen in a tool separate from protoc that's based on a different input format. There's a helper library as well.

In my position, I would like to see some stability in the third category of APIs, "Intended for use through a code-generation layer". It doesn't need to be as stable as the first two categories. My customers will be picking their own versions of gRPC--within a range that I support--so I cannot fix the version of gRPC that my codegen tool works against.

Is there a version/feature macro or something that I could use to detect these sort of changes so that my abstractions can be written to work across a range of gRPC versions? E.g., enabling me to write something akin to:

template <typename TResponse>
std::unique_ptr<grpc::ClientAsyncResponseReader <TResponse>>
MakeResponseReader(/* some arguments */)
{
    #if defined(GRPC_CODEGEN_HAS_INTERNAL_CREATE) // or maybe more like GRPC_CODEGEN_API >= 200000
        return std::unique_ptr<grpc::ClientAsyncResponseReader<TResponse>>(
            ::grpc::ClientAsyncResponseReader<TResponse>::internal::Create(/*args*/));
    #else
        return std::unique_ptr<grpc::ClientAsyncResponseReader<TResponse>>(
            ::grpc::ClientAsyncResponseReader<TResponse>::Create(/*args*/));
    #endif
}

Or, is there some other way to approach implementing clients and services without protoc that I should be exploring?

[vjpai]

Hi @chwarr , thanks for your message. That's actually a great suggestion, but we currently don't have a feature macro like that. We should add that for this reason and others. Currently, you can tell the version of gRPC by calling grpc::Version() . If you want to use this for conditional compilation, I would suggest building a small program that prints the version string as part of the Makefile and having that result set the #define's appropriately. At some point, though, we should add a macro to specify this.

[vjpai]

I would like to revive this gRFC. I'll be creating a pull request soon to bring it back. I'll see if I can address the reviewer comments on this pull request while doing so.