Skip to content

Conversation

@voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Aug 31, 2019

grpc-cronet relies on the feature of cronet's BidirectionalStream, which is created by ExperimentalCronetEngine#newBidirectionalStreamBuilder(...) (returns a ExperimentalBidirectionalStream.Builder). Since it's "experimental", we want to avoid calling APIs on ExperimentalBidirectionalStream.Builder.

We've removed grpc-cronet APIs that depends on ExperimentalBidirectionalStream.Builder's APIs and only allow restricted internal usages. External users should have no chance calling them and result in breakage. Now we switch to load those unstable ExperimentalBidirectionalStream.Builder APIs and invoke them through reflection at runtime.

The three potentially unstable APIs are setTrafficStatsTag, setTrafficStatsUid and addRequestAnnotation. For now, we handle method not found or method invocation exceptions by logging a warning and skip calling them. This handling should be tolerable and won't cause severe breakage. But later, we may choose to fail the RPC if failing to call those methods at runtime.

I'd expect the class ExperimentalBidirectionalStream.Builder to be there for stable as it is the return type of ExperimentalCronetEngine#newBidirectionalStreamBuilder(...). So we would not need to load ExperimentalBidirectionalStream.Builder class through reflection while only for invoking those less likely stable methods.

…tation methods for ExperimentalBidirectionalStream.Builder.
@voidzcy voidzcy requested review from ejona86 and removed request for ejona86 September 3, 2019 08:18
int tag) {
Exception caughtException = null;
try {
Method setTagMethod = ExperimentalBidirectionalStream.Builder.class
Copy link
Member

Choose a reason for hiding this comment

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

The getMethod() is the slowest part of reflection. We should save the result and re-use it across invoke()s.

Since we really aren't expecting this to ever fail (because the app will be shipping cronet itself), then performance for the failure case probably doesn't matter much. That means we could just re-call getMethod() each time if it is failing, and so we'd have an exception each time as well. We'd only need a single field to hold each Method.

We can use a lock or volatile to protect the Method field. It doesn't matter to me which.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@voidzcy voidzcy force-pushed the api/use_reflection_for_cronet_experimental_api_calls branch from f3ec779 to acef814 Compare September 5, 2019 21:44
@voidzcy voidzcy force-pushed the api/use_reflection_for_cronet_experimental_api_calls branch from acef814 to 4705599 Compare September 5, 2019 22:00
@voidzcy voidzcy requested a review from ejona86 September 6, 2019 07:54
@voidzcy voidzcy merged commit a234ada into grpc:master Sep 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants