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

api, core, services: make ProtoReflectionService interceptor compatible #6967

Merged

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Apr 23, 2020

Resolves #3469.

Major changes:

  • Put ServerImpl instance to its Context so that all server calls have access to the Server instance from which they are dispatched.
  • InternalNotifyOnServerBuild is eliminated as the server reference can be captured with a server interceptor.
  • Enhancement: allow using the same ProtoReflectionService instance across multiple servers without recomputing the whole service index for each server.

@voidzcy voidzcy marked this pull request as ready for review Apr 24, 2020
@voidzcy voidzcy requested review from ejona86 and ericgribkoff Apr 24, 2020
@@ -77,8 +77,7 @@
private ProtoReflectionService() {}

/**
* Creates a instance of {@link ProtoReflectionService}. Intended usage is one instance per
Copy link
Member

@ejona86 ejona86 Apr 30, 2020

Choose a reason for hiding this comment

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

Doh. I missed this before. Of course it was already limited to one server. Sorry for asking for the WeakHashMap, although it is nicer that way. I guess previously we'd have just returned wrong data? Yeah, using the WeakHashMap is much nicer for users.

@voidzcy
Copy link
Contributor Author

@voidzcy voidzcy commented Apr 30, 2020

Per offline discussion, we decided to expose Server through Context (with an internal key). So no new API needs to be added.

@voidzcy voidzcy changed the title api, core, services: make ProtoReflectionService interceptor compatible core, services: make ProtoReflectionService interceptor compatible Apr 30, 2020
@voidzcy voidzcy requested a review from ejona86 Apr 30, 2020
*/
@Internal
public class InternalServerAccessor {
public static final Context.Key<Server> SERVER_KEY = ServerImpl.SERVER_CONTEXT_KEY;
Copy link
Member

@ejona86 ejona86 Apr 30, 2020

Choose a reason for hiding this comment

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

Since this instance is being used as API and not as convenience, the instance needs to be created in io.grpc (not just copied, like here). If we shaded io.grpc.internal this reference would no longer match between implementations.

Copy link
Contributor Author

@voidzcy voidzcy Apr 30, 2020

Choose a reason for hiding this comment

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

Moved the creation to io.grpc.Server and accessor to io.grpc.

/**
* Internal accessor for getting the {@link Server} instance inside server RPC {@link Context}.
* This is intended for usage internal to the gRPC team, as it's unclear to us what users would
* need. If you *really* think you need to use this, please file an issue for us to discuss a
Copy link
Member

@ejona86 ejona86 Apr 30, 2020

Choose a reason for hiding this comment

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

Let's drop the "really". I don't see the need for the emphasis, and no real need to scare off users from filing an issue.

Copy link
Contributor Author

@voidzcy voidzcy Apr 30, 2020

Choose a reason for hiding this comment

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

Reworded. Users should refer to the documentation at where the Context key is created (io.grpc.Server). The accessor is just for our own usage.

Copy link
Member

@ejona86 ejona86 left a comment

After this one change, it will probably be finished.

* unclear to us what users would need. If you think you need to use this, please file an
* issue for us to discuss a public API.
*/
protected static final Context.Key<Server> SERVER_CONTEXT_KEY =
Copy link
Member

@ejona86 ejona86 Apr 30, 2020

Choose a reason for hiding this comment

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

protected leaks it. Using package-private is probably better.

voidzcy
Copy link
Contributor Author

@voidzcy voidzcy commented on 9a979c5 Apr 30, 2020

Choose a reason for hiding this comment

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

Okay, I was trying to have this usable in ServerImpl.

Made this package-private and changed that to use the accessor.

Copy link
Member

@ejona86 ejona86 left a comment

Thank you!

@voidzcy voidzcy changed the title core, services: make ProtoReflectionService interceptor compatible api, core, services: make ProtoReflectionService interceptor compatible May 1, 2020
@voidzcy voidzcy merged commit a423900 into grpc:master May 1, 2020
13 checks passed
ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 18, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Motivations:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 18, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Motivations:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes line#2806
trustin pushed a commit to line/armeria that referenced this issue Jun 19, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 #2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Modifications:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes #2806
filipeGuerreiro pushed a commit to filipeGuerreiro/armeria that referenced this issue Sep 19, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Modifications:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes line#2806
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
…le (grpc#6967)

Eliminate the hack of InternalNotifyOnBuild mechanism for letting ProtoReflectionService get access to the Sever instance, which makes ProtoReflectionService incompatible with server interceptors. This change put the Server instance into the Context and let the ProtoReflectionService RPC obtain it in its RPC Context. Also enhanced ProtoReflectionService so that one service instance can be used across multiple servers.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2021
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