binder: Pre-factor out the guts of the BinderClientTransport handshake. #12292
Conversation
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
…-java into isolated-process-2
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
…-java into isolated-process-2
… into isolated-process-2
|
@mateusazis PTAL |
binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java
Outdated
Show resolved
Hide resolved
| int remoteUid = Binder.getCallingUid(); | ||
| restrictIncomingBinderToCallsFrom(remoteUid); | ||
| attributes = setSecurityAttrs(attributes, remoteUid); | ||
| authResultFuture = checkServerAuthorizationAsync(remoteUid); |
There was a problem hiding this comment.
Not sure about the plans for the next PR, but it seems like authResultFuture is never used by the parent class anymore except during cancellation.
It feels a bit weird to see one class setting another one's fields, even if they are nested. Could we move authResultFuture into this one and add a cancel method to ClientHandshake?
Ditto possibly for attributes; couldn't fully track all the places where it is used.
There was a problem hiding this comment.
The other ClientHandshake impl will also need to set and potentially cancel authResultFuture so I'd like to keep this member in some shared location. Fortunately I expect this issue will be going away soon with my proposed fix for #12283 (jdcormie@d0bcf4b). LMK if that would be satisfactory.
As for attributes, deciding how and when to update the transport attributes with the peer's UID is one of the core responsibilities of ClientHandshake. As I mentioned in the other comment, the relationship between BinderClientTransport and the ClientHandshake impls is intentionally intimate. My goal with ClientHandshake isn't actually to create an arm's length relationship with the transport, rather it's just to avoid writing if (flag) { /* v1 */ } else { /* v2 */} all over the place. LMK if you feel strongly and/or suggest a different approach.
There was a problem hiding this comment.
Re: "plans for the next PR" I just uploaded it as #12398. However, I recommend reviewing it as CR/814340095 which will let you ignore the diffbase (CR/808247923).
The critique view of this PR (CR/808247923) also does a much better job of showing how this PR is really just moving code around without changing it.
binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java
Outdated
Show resolved
Hide resolved
Make ClientHandshake an interface Make the impl private/final
Part 1 of fixing #12293. No behavior change intended this is a pure refactor.