Implement missing authority API for HandlerRegistry #2709

rmichela · 2017-02-08T00:56:26Z

HandlerRegistry.lookupMethod(String methodName, String authority) is never never invoked with a non-null authority string. Per the discussion on the mailing list, I implemented this method.

https://groups.google.com/forum/#!topic/grpc-io/1RPka9CKAvE

grpc-kokoro · 2017-02-08T00:56:27Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

googlebot · 2017-02-08T00:56:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

rmichela · 2017-02-08T00:59:02Z

@ejona86 I've implemented this PR per your recommendations on IRC.

ejona86

Looks pretty good. Only small changes necessary.

It could use two tests or so.

ejona86 · 2017-02-08T20:49:43Z

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

  @GuardedBy("this")
  private Set<InProcessStream> streams = new HashSet<InProcessStream>();
+  @GuardedBy("this")
+  private String authority;


Make this final, and no need for the @GuardedBy then. (See comment below about the setter)

ejona86 · 2017-02-08T20:50:53Z

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

      public void setAuthority(String string) {
-        // TODO(ejona): Do something with this? Could be useful for testing, but can we "validate"
-        // it?
+        authority = string;


This set only impacts this one stream. So add another field for the authority to the stream. You can initialize it from the transport's value.

ejona86 · 2017-02-08T20:54:15Z

netty/src/main/java/io/grpc/netty/NettyServerHandler.java

+  private String getOrUpdateAuthority(AsciiString authority) {
+    if (authority == null) {
+      return "";
+    } else if (lastKnownAuthority == null || !lastKnownAuthority.equals(authority)) {


If you invert the equals check you can avoid the null check. !authority.equals(lastKnownAuthority)

ejona86 · 2017-02-08T21:25:43Z

core/src/main/java/io/grpc/internal/AbstractServerStream.java

  }

+  @Override
+  public String getAuthority() {


Given that HandlerRegistry is the only thing that is using the value from getAuthority() currently, and it is @Nullable, I would still feel better by defaulting to null. Here and in NettyServerHandler.

It's something we can change in the future though. We'll probably deal with it when we expose getAuthority() on ServerCall. At that point we may make it non-null where empty string is explicitly called out in the API.

rmichela · 2017-02-08T21:45:32Z

I signed it!

googlebot · 2017-02-08T21:45:33Z

CLAs look good, thanks!

carl-mastrangelo · 2017-02-08T23:09:07Z

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

  private Set<InProcessStream> streams = new HashSet<InProcessStream>();

-  public InProcessTransport(String name) {
+  public InProcessTransport(String name, String authority) {


This would be API breaking.

No, it isn't. InProcessTransport itself is package-private.

carl-mastrangelo · 2017-02-08T23:09:42Z

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

      public void setAuthority(String string) {
-        // TODO(ejona): Do something with this? Could be useful for testing, but can we "validate"
-        // it?
+        InProcessStream.this.authority = string;


This would not be thread safe

InProcessStream.this.authority is now volatile. Setting and reading references are atomic in Java. Strings are immutable. This should be thread safe now, right?

Actually, it was fine as it was before (without volatile). setAuthority "May only be called before start."

rmichela · 2017-02-09T01:24:30Z

@ejona86 @carl-mastrangelo Test cases added and feedback addressed.

rmichela · 2017-02-09T18:10:15Z

I can't reproduce the serverNotListening test failure. I think it's a flapping test.

ejona86 · 2017-02-09T22:53:59Z

Jenkins, okay to test.

ejona86 · 2017-02-10T18:01:42Z

okay to test

carl-mastrangelo · 2017-02-10T18:10:02Z

netty/src/main/java/io/grpc/netty/NettyServerHandler.java

+      lastKnownAuthority = authority;
+    }
+
+    // AsciiString.toString() is internally cached, so subsequent calls will not


This is not efficient enough. If the client changes the authority every request and the server doesn't read from it, it will unnecessarily convert it. Why not do the conversion when getAuthority is called?

getAuthority() is always called.

This is plenty efficient enough. It doesn't hinder the common case at all.

Most clients, by far, won't change the authority every request. And even if they do change constantly, then any optimization here would probably be in the noise.

I'd rather get to a point we can call good enough for now and any future optimization for edge cases gets included with the HPACK tricks we're planning.

I talked with Carl. He showed me that getAuthority() isn't always called, since it is only called when the immutable lookup fails. But we also agreed that it will 99% of the time always be the same authority. And we also believe that the HPACK table will even make it the same AsciiString instance. So Carl is now okay with this. He would still like to have our nice HPACK stuff, but that's orthogonal and future work.

ejona86 · 2017-02-11T01:20:00Z

Thank you for implementing this! I'm sorry that I didn't do it initially, but I'm glad you were able to.

Implement missing authority API for HandlerRegistry

1e98d8c

rmichela mentioned this pull request Feb 8, 2017

Implement missing authority API for HandlerRegistry #2704

Closed

ejona86 reviewed Feb 8, 2017

View reviewed changes

Absent authority should be null, not empty string

a47f26f

carl-mastrangelo reviewed Feb 8, 2017

View reviewed changes

rmichela added 2 commits February 8, 2017 15:45

Don't break InProcessTransport

2d149a7

getAuthority() unit tests

032e0e2

ejona86 approved these changes Feb 10, 2017

View reviewed changes

carl-mastrangelo reviewed Feb 10, 2017

View reviewed changes

ejona86 merged commit 4029b3f into grpc:master Feb 11, 2017

rmichela deleted the feature/HandlerRegistry-authority2 branch February 13, 2017 17:33

lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

Implement missing authority API for HandlerRegistry #2709

Implement missing authority API for HandlerRegistry #2709

Uh oh!

Conversation

rmichela commented Feb 8, 2017

Uh oh!

grpc-kokoro commented Feb 8, 2017

Uh oh!

googlebot commented Feb 8, 2017

Uh oh!

rmichela commented Feb 8, 2017

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmichela commented Feb 8, 2017

Uh oh!

googlebot commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmichela commented Feb 9, 2017

Uh oh!

rmichela commented Feb 9, 2017

Uh oh!

ejona86 commented Feb 9, 2017

Uh oh!

ejona86 commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants