From 73dd1e65fa1d6796a4e82650dbd3e8ec25e9da01 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 28 Oct 2025 11:37:44 -0700 Subject: [PATCH 1/5] Flatten BinderClientTransport#start using the "guard clause" pattern. The existing if/else style of error handling is hard to read and hard to change. https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html This is a pure refactor -- No behavior change. --- .../internal/BinderClientTransport.java | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index c6aa195544e..aca048128e6 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -148,31 +148,33 @@ public synchronized void onUnbound(Status reason) { @Override public synchronized Runnable start(Listener clientTransportListener) { this.clientTransportListener = checkNotNull(clientTransportListener); - return () -> { - synchronized (BinderClientTransport.this) { - if (inState(TransportState.NOT_STARTED)) { - setState(TransportState.SETUP); - try { - if (preAuthorizeServer) { - preAuthorize(serviceBinding.resolve()); - } else { - serviceBinding.bind(); - } - } catch (StatusException e) { - shutdownInternal(e.getStatus(), true); - return; - } - if (readyTimeoutMillis >= 0) { - readyTimeoutFuture = - getScheduledExecutorService() - .schedule( - BinderClientTransport.this::onReadyTimeout, - readyTimeoutMillis, - MILLISECONDS); - } - } + return this::postStartRunnable; + } + + private synchronized void postStartRunnable() { + if (!inState(TransportState.NOT_STARTED)) { + return; + } + + setState(TransportState.SETUP); + + try { + if (preAuthorizeServer) { + preAuthorize(serviceBinding.resolve()); + } else { + serviceBinding.bind(); } - }; + } catch (StatusException e) { + shutdownInternal(e.getStatus(), true); + return; + } + + if (readyTimeoutMillis >= 0) { + readyTimeoutFuture = + getScheduledExecutorService() + .schedule( + BinderClientTransport.this::onReadyTimeout, readyTimeoutMillis, MILLISECONDS); + } } @GuardedBy("this") From 299ac1376d02d91786390724a4ed85f633f7e19b Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 28 Oct 2025 11:43:39 -0700 Subject: [PATCH 2/5] Flatten BinderClientTransport#handleAuthResult using the "guard clause" pattern. The existing if/else style of error handling is hard to read and hard to change: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html This is a pure refactor -- No behavior change. --- .../internal/BinderClientTransport.java | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index aca048128e6..5dd0047d46b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -206,13 +206,16 @@ public void onFailure(Throwable t) { } private synchronized void handlePreAuthResult(Status authorization) { - if (inState(TransportState.SETUP)) { - if (!authorization.isOk()) { - shutdownInternal(authorization, true); - } else { - serviceBinding.bind(); - } + if (!inState(TransportState.SETUP)) { + return; } + + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + return; + } + + serviceBinding.bind(); } private synchronized void onReadyTimeout() { @@ -358,18 +361,21 @@ private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { } private synchronized void handleAuthResult(Status authorization) { - if (inState(TransportState.SETUP)) { - if (!authorization.isOk()) { - shutdownInternal(authorization, true); - } else { - setState(TransportState.READY); - attributes = clientTransportListener.filterTransport(attributes); - clientTransportListener.transportReady(); - if (readyTimeoutFuture != null) { - readyTimeoutFuture.cancel(false); - readyTimeoutFuture = null; - } - } + if (!inState(TransportState.SETUP)) { + return; + } + + if (!authorization.isOk()) { + shutdownInternal(authorization, true); + return; + } + + setState(TransportState.READY); + attributes = clientTransportListener.filterTransport(attributes); + clientTransportListener.transportReady(); + if (readyTimeoutFuture != null) { + readyTimeoutFuture.cancel(false); + readyTimeoutFuture = null; } } From ad8155fec2cae6057529dfd922ce1d002bd30a35 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 28 Oct 2025 11:48:29 -0700 Subject: [PATCH 3/5] Flatten BinderClientTransport#handleSetupTransport using the "guard clause" pattern. The existing if/else style of error handling is hard to read and hard to change: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html This is a pure refactor -- No behavior change. --- .../internal/BinderClientTransport.java | 71 ++++++++++--------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 5dd0047d46b..0669c53c29a 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -319,39 +319,46 @@ void notifyTerminated() { @Override @GuardedBy("this") protected void handleSetupTransport(Parcel parcel) { - int remoteUid = Binder.getCallingUid(); - if (inState(TransportState.SETUP)) { - int version = parcel.readInt(); - IBinder binder = parcel.readStrongBinder(); - if (version != WIRE_FORMAT_VERSION) { - shutdownInternal(Status.UNAVAILABLE.withDescription("Wire format version mismatch"), true); - } else if (binder == null) { - shutdownInternal( - Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); - } else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { - shutdownInternal( - Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); - } else { - restrictIncomingBinderToCallsFrom(remoteUid); - attributes = setSecurityAttrs(attributes, remoteUid); - ListenableFuture authResultFuture = - register(checkServerAuthorizationAsync(remoteUid)); - Futures.addCallback( - authResultFuture, - new FutureCallback() { - @Override - public void onSuccess(Status result) { - handleAuthResult(result); - } - - @Override - public void onFailure(Throwable t) { - handleAuthResult(t); - } - }, - offloadExecutor); - } + if (!inState(TransportState.SETUP)) { + return; + } + + int version = parcel.readInt(); + if (version != WIRE_FORMAT_VERSION) { + shutdownInternal(Status.UNAVAILABLE.withDescription("Wire format version mismatch"), true); + return; + } + + IBinder binder = parcel.readStrongBinder(); + if (binder == null) { + shutdownInternal(Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); + return; + } + + if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) { + shutdownInternal( + Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true); + return; } + + int remoteUid = Binder.getCallingUid(); + restrictIncomingBinderToCallsFrom(remoteUid); + attributes = setSecurityAttrs(attributes, remoteUid); + ListenableFuture authResultFuture = register(checkServerAuthorizationAsync(remoteUid)); + Futures.addCallback( + authResultFuture, + new FutureCallback() { + @Override + public void onSuccess(Status result) { + handleAuthResult(result); + } + + @Override + public void onFailure(Throwable t) { + handleAuthResult(t); + } + }, + offloadExecutor); } private ListenableFuture checkServerAuthorizationAsync(int remoteUid) { From 619dfaebc787ffbfa62816beda54dcdbabc56bfb Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 28 Oct 2025 11:52:16 -0700 Subject: [PATCH 4/5] Flatten BinderClientTransport#newStream using the "guard clause" pattern. The existing if/else style of error handling is hard to read and hard to change: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html This is a pure refactor -- No behavior change. --- .../internal/BinderClientTransport.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 0669c53c29a..54d65936fab 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -257,17 +257,17 @@ public synchronized ClientStream newStream( Status failure = Status.INTERNAL.withDescription("Clashing call IDs"); shutdownInternal(failure, true); return newFailingClientStream(failure, attributes, headers, tracers); + } + + if (inbound.countsForInUse() && numInUseStreams.getAndIncrement() == 0) { + clientTransportListener.transportInUse(true); + } + Outbound.ClientOutbound outbound = + new Outbound.ClientOutbound(this, callId, method, headers, statsTraceContext); + if (method.getType().clientSendsOneMessage()) { + return new SingleMessageClientStream(inbound, outbound, attributes); } else { - if (inbound.countsForInUse() && numInUseStreams.getAndIncrement() == 0) { - clientTransportListener.transportInUse(true); - } - Outbound.ClientOutbound outbound = - new Outbound.ClientOutbound(this, callId, method, headers, statsTraceContext); - if (method.getType().clientSendsOneMessage()) { - return new SingleMessageClientStream(inbound, outbound, attributes); - } else { - return new MultiMessageClientStream(inbound, outbound, attributes); - } + return new MultiMessageClientStream(inbound, outbound, attributes); } } From c96d7de24686f700af52a6e982da9bdedbca40be Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 28 Oct 2025 11:56:25 -0700 Subject: [PATCH 5/5] Flatten BinderServerTransport using the "guard clause" pattern. The existing if/else style of error handling is hard to read and hard to change: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html This is a pure refactor -- No behavior change. --- .../internal/BinderServerTransport.java | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java index cbe64149e15..44909f9c0bc 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServerTransport.java @@ -69,15 +69,22 @@ public BinderServerTransport( */ public synchronized void start(ServerTransportListener serverTransportListener) { this.listenerPromise.set(serverTransportListener); - if (!isShutdown()) { - sendSetupTransaction(); - // Check we're not shutdown again, since a failure inside sendSetupTransaction (or a callback - // it triggers), could have shut us down. - if (!isShutdown()) { - setState(TransportState.READY); - attributes = serverTransportListener.transportReady(attributes); - } + if (isShutdown()) { + // It's unlikely, but we could be shutdown externally between construction and start(). One + // possible cause is an extremely short handshake timeout. + return; } + + sendSetupTransaction(); + + // Check we're not shutdown again, since a failure inside sendSetupTransaction (or a callback + // it triggers), could have shut us down. + if (isShutdown()) { + return; + } + + setState(TransportState.READY); + attributes = serverTransportListener.transportReady(attributes); } StatsTraceContext createStatsTraceContext(String methodName, Metadata headers) { @@ -92,10 +99,10 @@ StatsTraceContext createStatsTraceContext(String methodName, Metadata headers) { synchronized Status startStream(ServerStream stream, String methodName, Metadata headers) { if (isShutdown()) { return Status.UNAVAILABLE.withDescription("transport is shutdown"); - } else { - listenerPromise.get().streamCreated(stream, methodName, headers); - return Status.OK; } + + listenerPromise.get().streamCreated(stream, methodName, headers); + return Status.OK; } @Override