stub: Add disableAutoRequestWithInitial that disables all automatic inbound flow-control requests

Add a new disableAutoRequest method that disables all automatic requests while disableAutoInboundFlowControl maintains existing behavior.

The default behavior of requesting initial messages is applied even if disableAutoInboundFlowControl is called. ServerCalls disables all automatic flow control which is much more useful in case the user can't handle incoming messages until some time after the call has started.  This change creates a new StartableListener that has an onStart method that is invoked when the call is started which makes initial requests if necessary.

See #6806
This commit is contained in:
DRayX 2020-05-06 10:19:41 -07:00 committed by GitHub
parent 0057c4f29d
commit a9250c1f99
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 219 additions and 83 deletions

View File

@ -54,7 +54,7 @@ public class ManualFlowControlClient {
this.requestStream = requestStream;
// Set up manual flow control for the response stream. It feels backwards to configure the response
// stream's flow control using the request stream's observer, but this is the way it is.
requestStream.disableAutoInboundFlowControl();
requestStream.disableAutoRequestWithInitial(1);
// Set up a back-pressure-aware producer for the request stream. The onReadyHandler will be invoked
// when the consuming side has enough buffer space to receive more messages.

View File

@ -39,7 +39,7 @@ public class ManualFlowControlServer {
// stream's flow control using the response stream's observer, but this is the way it is.
final ServerCallStreamObserver<HelloReply> serverCallStreamObserver =
(ServerCallStreamObserver<HelloReply>) responseObserver;
serverCallStreamObserver.disableAutoInboundFlowControl();
serverCallStreamObserver.disableAutoRequestWithInitial(0);
// Set up a back-pressure-aware consumer for the request stream. The onReadyHandler will be invoked
// when the consuming side has enough buffer space to receive more messages.

View File

@ -133,8 +133,7 @@ public final class ProtoReflectionService extends ServerReflectionGrpc.ServerRef
ProtoReflectionStreamObserver requestObserver =
new ProtoReflectionStreamObserver(getRefreshedIndex(), serverCallStreamObserver);
serverCallStreamObserver.setOnReadyHandler(requestObserver);
serverCallStreamObserver.disableAutoInboundFlowControl();
serverCallStreamObserver.request(1);
serverCallStreamObserver.disableAutoRequestWithInitial(1);
return requestObserver;
}

View File

@ -564,8 +564,11 @@ public class ProtoReflectionServiceTest {
(ClientCallStreamObserver<ServerReflectionRequest>)
stub.serverReflectionInfo(clientResponseObserver);
// ClientCalls.startCall() calls request(1) initially, so we should get an immediate response.
// Verify we don't receive a response until we request it.
requestObserver.onNext(flowControlRequest);
assertEquals(0, clientResponseObserver.getResponses().size());
requestObserver.request(1);
assertEquals(1, clientResponseObserver.getResponses().size());
assertEquals(flowControlGoldenResponse, clientResponseObserver.getResponses().get(0));
@ -589,17 +592,15 @@ public class ProtoReflectionServiceTest {
(ClientCallStreamObserver<ServerReflectionRequest>)
stub.serverReflectionInfo(clientResponseObserver);
// ClientCalls.startCall() calls request(1) initially, so make additional request.
requestObserver.onNext(flowControlRequest);
requestObserver.onNext(flowControlRequest);
requestObserver.onCompleted();
assertEquals(1, clientResponseObserver.getResponses().size());
assertEquals(0, clientResponseObserver.getResponses().size());
assertFalse(clientResponseObserver.onCompleteCalled());
requestObserver.request(1);
assertTrue(clientResponseObserver.onCompleteCalled());
assertEquals(2, clientResponseObserver.getResponses().size());
assertEquals(flowControlGoldenResponse, clientResponseObserver.getResponses().get(1));
assertEquals(1, clientResponseObserver.getResponses().size());
assertEquals(flowControlGoldenResponse, clientResponseObserver.getResponses().get(0));
}
private final ServerReflectionRequest flowControlRequest =
@ -626,7 +627,7 @@ public class ProtoReflectionServiceTest {
@Override
public void beforeStart(final ClientCallStreamObserver<ServerReflectionRequest> requestStream) {
requestStream.disableAutoInboundFlowControl();
requestStream.disableAutoRequestWithInitial(0);
}
@Override

View File

@ -106,9 +106,37 @@ public abstract class CallStreamObserver<V> implements StreamObserver<V> {
* </li>
* </ul>
* </p>
*
* <p>To migrate to {@link #disableAutoRequestWithInitial} on the server side, call
* {@code disableAutoRequestWithInitial(0)} as {@code disableAutoInboundFlowControl}
* already disables all inbound requests. On the client side, {@code
* disableAutoRequestWithInitial(1)} should be called to maintain existing behavior as
* {@code disableAutoInboundFlowControl} does not disable the initial request.
*/
public abstract void disableAutoInboundFlowControl();
/**
* Disables automatic flow control where an additional message is requested to be read after a
* call to the 'inbound' {@link io.grpc.stub.StreamObserver#onNext(Object)} has completed. A
* number of initial requests to make when the call is started may be specified.
*
* <p>On client-side this method may only be called during {@link
* ClientResponseObserver#beforeStart}. On server-side it may only be called during the initial
* call to the application, before the service returns its {@code StreamObserver}.
*
* <p>Note that for server-side cases where the message is received before the handler is invoked,
* this method will have no effect. This is true for:
*
* <ul>
* <li>{@link io.grpc.MethodDescriptor.MethodType#UNARY} operations.</li>
* <li>{@link io.grpc.MethodDescriptor.MethodType#SERVER_STREAMING} operations.</li>
* </ul>
* </p>
*
* <p>This API is still a work in-progress and will likely change in the future.
*/
public abstract void disableAutoRequestWithInitial(int request);
/**
* Requests the peer to produce {@code count} more messages to be delivered to the 'inbound'
* {@link StreamObserver}.

View File

@ -164,7 +164,7 @@ public final class ClientCalls {
public static <ReqT, RespT> Iterator<RespT> blockingServerStreamingCall(
ClientCall<ReqT, RespT> call, ReqT req) {
BlockingResponseStream<RespT> result = new BlockingResponseStream<>(call);
asyncUnaryRequestCall(call, req, result.listener(), true);
asyncUnaryRequestCall(call, req, result.listener());
return result;
}
@ -183,7 +183,7 @@ public final class ClientCalls {
callOptions.withOption(ClientCalls.STUB_TYPE_OPTION, StubType.BLOCKING)
.withExecutor(executor));
BlockingResponseStream<RespT> result = new BlockingResponseStream<>(call, executor);
asyncUnaryRequestCall(call, req, result.listener(), true);
asyncUnaryRequestCall(call, req, result.listener());
return result;
}
@ -197,7 +197,7 @@ public final class ClientCalls {
public static <ReqT, RespT> ListenableFuture<RespT> futureUnaryCall(
ClientCall<ReqT, RespT> call, ReqT req) {
GrpcFuture<RespT> responseFuture = new GrpcFuture<>(call);
asyncUnaryRequestCall(call, req, new UnaryStreamToFuture<>(responseFuture), false);
asyncUnaryRequestCall(call, req, new UnaryStreamToFuture<>(responseFuture));
return responseFuture;
}
@ -278,17 +278,14 @@ public final class ClientCalls {
req,
new StreamObserverToCallListenerAdapter<>(
responseObserver,
new CallToStreamObserverAdapter<>(call),
streamingResponse),
streamingResponse);
new CallToStreamObserverAdapter<>(call, streamingResponse)));
}
private static <ReqT, RespT> void asyncUnaryRequestCall(
ClientCall<ReqT, RespT> call,
ReqT req,
ClientCall.Listener<RespT> responseListener,
boolean streamingResponse) {
startCall(call, responseListener, streamingResponse);
StartableListener<RespT> responseListener) {
startCall(call, responseListener);
try {
call.sendMessage(req);
call.halfClose();
@ -303,40 +300,39 @@ public final class ClientCalls {
ClientCall<ReqT, RespT> call,
StreamObserver<RespT> responseObserver,
boolean streamingResponse) {
CallToStreamObserverAdapter<ReqT> adapter = new CallToStreamObserverAdapter<>(call);
CallToStreamObserverAdapter<ReqT> adapter = new CallToStreamObserverAdapter<>(
call, streamingResponse);
startCall(
call,
new StreamObserverToCallListenerAdapter<>(
responseObserver, adapter, streamingResponse),
streamingResponse);
new StreamObserverToCallListenerAdapter<>(responseObserver, adapter));
return adapter;
}
private static <ReqT, RespT> void startCall(
ClientCall<ReqT, RespT> call,
ClientCall.Listener<RespT> responseListener,
boolean streamingResponse) {
StartableListener<RespT> responseListener) {
call.start(responseListener, new Metadata());
if (streamingResponse) {
call.request(1);
} else {
// Initially ask for two responses from flow-control so that if a misbehaving server sends
// more than one responses, we can catch it and fail it in the listener.
call.request(2);
}
responseListener.onStart();
}
private abstract static class StartableListener<T> extends ClientCall.Listener<T> {
abstract void onStart();
}
private static final class CallToStreamObserverAdapter<T> extends ClientCallStreamObserver<T> {
private boolean frozen;
private final ClientCall<T, ?> call;
private final boolean streamingResponse;
private Runnable onReadyHandler;
private boolean autoFlowControlEnabled = true;
private int initialRequest = 1;
private boolean autoRequestEnabled = true;
private boolean aborted = false;
private boolean completed = false;
// Non private to avoid synthetic class
CallToStreamObserverAdapter(ClientCall<T, ?> call) {
CallToStreamObserverAdapter(ClientCall<T, ?> call, boolean streamingResponse) {
this.call = call;
this.streamingResponse = streamingResponse;
}
private void freeze() {
@ -376,18 +372,32 @@ public final class ClientCalls {
this.onReadyHandler = onReadyHandler;
}
@Deprecated
@Override
public void disableAutoInboundFlowControl() {
disableAutoRequestWithInitial(1);
}
@Override
public void disableAutoRequestWithInitial(int request) {
if (frozen) {
throw new IllegalStateException(
"Cannot disable auto flow control after call started. Use ClientResponseObserver");
}
autoFlowControlEnabled = false;
Preconditions.checkArgument(request >= 0, "Initial requests must be non-negative");
initialRequest = request;
autoRequestEnabled = false;
}
@Override
public void request(int count) {
call.request(count);
if (!streamingResponse && count == 1) {
// Initially ask for two responses from flow-control so that if a misbehaving server
// sends more than one responses, we can catch it and fail it in the listener.
call.request(2);
} else {
call.request(count);
}
}
@Override
@ -402,19 +412,16 @@ public final class ClientCalls {
}
private static final class StreamObserverToCallListenerAdapter<ReqT, RespT>
extends ClientCall.Listener<RespT> {
extends StartableListener<RespT> {
private final StreamObserver<RespT> observer;
private final CallToStreamObserverAdapter<ReqT> adapter;
private final boolean streamingResponse;
private boolean firstResponseReceived;
// Non private to avoid synthetic class
StreamObserverToCallListenerAdapter(
StreamObserver<RespT> observer,
CallToStreamObserverAdapter<ReqT> adapter,
boolean streamingResponse) {
CallToStreamObserverAdapter<ReqT> adapter) {
this.observer = observer;
this.streamingResponse = streamingResponse;
this.adapter = adapter;
if (observer instanceof ClientResponseObserver) {
@SuppressWarnings("unchecked")
@ -431,7 +438,7 @@ public final class ClientCalls {
@Override
public void onMessage(RespT message) {
if (firstResponseReceived && !streamingResponse) {
if (firstResponseReceived && !adapter.streamingResponse) {
throw Status.INTERNAL
.withDescription("More than one responses received for unary or client-streaming call")
.asRuntimeException();
@ -439,7 +446,7 @@ public final class ClientCalls {
firstResponseReceived = true;
observer.onNext(message);
if (streamingResponse && adapter.autoFlowControlEnabled) {
if (adapter.streamingResponse && adapter.autoRequestEnabled) {
// Request delivery of the next inbound message.
adapter.request(1);
}
@ -460,12 +467,19 @@ public final class ClientCalls {
adapter.onReadyHandler.run();
}
}
@Override
void onStart() {
if (adapter.initialRequest > 0) {
adapter.request(adapter.initialRequest);
}
}
}
/**
* Completes a {@link GrpcFuture} using {@link StreamObserver} events.
*/
private static final class UnaryStreamToFuture<RespT> extends ClientCall.Listener<RespT> {
private static final class UnaryStreamToFuture<RespT> extends StartableListener<RespT> {
private final GrpcFuture<RespT> responseFuture;
private RespT value;
@ -501,6 +515,11 @@ public final class ClientCalls {
responseFuture.setException(status.asRuntimeException(trailers));
}
}
@Override
void onStart() {
responseFuture.call.request(2);
}
}
private static final class GrpcFuture<RespT> extends AbstractFuture<RespT> {
@ -542,7 +561,7 @@ public final class ClientCalls {
private static final class BlockingResponseStream<T> implements Iterator<T> {
// Due to flow control, only needs to hold up to 2 items: 1 for value, 1 for close.
private final BlockingQueue<Object> buffer = new ArrayBlockingQueue<>(2);
private final ClientCall.Listener<T> listener = new QueuingListener();
private final StartableListener<T> listener = new QueuingListener();
private final ClientCall<?, T> call;
/** May be null. */
private final ThreadlessExecutor threadless;
@ -560,7 +579,7 @@ public final class ClientCalls {
this.threadless = threadless;
}
ClientCall.Listener<T> listener() {
StartableListener<T> listener() {
return listener;
}
@ -632,7 +651,7 @@ public final class ClientCalls {
throw new UnsupportedOperationException();
}
private final class QueuingListener extends ClientCall.Listener<T> {
private final class QueuingListener extends StartableListener<T> {
// Non private to avoid synthetic class
QueuingListener() {}
@ -658,6 +677,11 @@ public final class ClientCalls {
}
done = true;
}
@Override
void onStart() {
call.request(1);
}
}
}

View File

@ -30,8 +30,8 @@ public interface ClientResponseObserver<ReqT, RespT> extends StreamObserver<Resp
* onReady events, disable auto inbound flow and perform other advanced functions.
*
* <p>Only the methods {@link ClientCallStreamObserver#setOnReadyHandler(Runnable)} and
* {@link ClientCallStreamObserver#disableAutoInboundFlowControl()} may be called within this
* callback
* {@link ClientCallStreamObserver#disableAutoRequestWithInitial(int)} may be called within
* this callback
*
* <pre>
* // Copy an iterator to the request stream under flow-control

View File

@ -223,8 +223,8 @@ public final class ServerCalls {
new ServerCallStreamObserverImpl<>(call);
StreamObserver<ReqT> requestObserver = method.invoke(responseObserver);
responseObserver.freeze();
if (responseObserver.autoFlowControlEnabled) {
call.request(1);
if (responseObserver.initialRequest > 0) {
call.request(responseObserver.initialRequest);
}
return new StreamingServerCallListener(requestObserver, responseObserver, call);
}
@ -251,7 +251,7 @@ public final class ServerCalls {
requestObserver.onNext(request);
// Request delivery of the next inbound message.
if (responseObserver.autoFlowControlEnabled) {
if (responseObserver.autoRequestEnabled) {
call.request(1);
}
}
@ -308,7 +308,8 @@ public final class ServerCalls {
final ServerCall<ReqT, RespT> call;
volatile boolean cancelled;
private boolean frozen;
private boolean autoFlowControlEnabled = true;
private int initialRequest = 1;
private boolean autoRequestEnabled = true;
private boolean sentHeaders;
private Runnable onReadyHandler;
private Runnable onCancelHandler;
@ -399,10 +400,18 @@ public final class ServerCalls {
this.onCancelHandler = onCancelHandler;
}
@Deprecated
@Override
public void disableAutoInboundFlowControl() {
disableAutoRequestWithInitial(0);
}
@Override
public void disableAutoRequestWithInitial(int request) {
checkState(!frozen, "Cannot disable auto flow control after initialization");
autoFlowControlEnabled = false;
Preconditions.checkArgument(request >= 0, "Initial requests must be non-negative");
initialRequest = request;
autoRequestEnabled = false;
}
@Override

View File

@ -412,6 +412,48 @@ public class ClientCallsTest {
assertThat(requests).containsExactly(1);
}
@Test
public void disablingAutoRequestSuppressesRequests()
throws Exception {
final AtomicReference<ClientCall.Listener<String>> listener =
new AtomicReference<>();
final List<Integer> requests = new ArrayList<>();
NoopClientCall<Integer, String> call = new NoopClientCall<Integer, String>() {
@Override
public void start(io.grpc.ClientCall.Listener<String> responseListener, Metadata headers) {
listener.set(responseListener);
}
@Override
public void request(int numMessages) {
requests.add(numMessages);
}
};
ClientCalls.asyncBidiStreamingCall(call, new ClientResponseObserver<Integer, String>() {
@Override
public void beforeStart(ClientCallStreamObserver<Integer> requestStream) {
requestStream.disableAutoRequestWithInitial(0);
}
@Override
public void onNext(String value) {
}
@Override
public void onError(Throwable t) {
}
@Override
public void onCompleted() {
}
});
listener.get().onMessage("message");
assertThat(requests).isEmpty();
}
@Test
public void callStreamObserverPropagatesFlowControlRequestsToCall()
throws Exception {
@ -419,7 +461,7 @@ public class ClientCallsTest {
new ClientResponseObserver<Integer, String>() {
@Override
public void beforeStart(ClientCallStreamObserver<Integer> requestStream) {
requestStream.disableAutoInboundFlowControl();
requestStream.disableAutoRequestWithInitial(0);
}
@Override
@ -460,26 +502,34 @@ public class ClientCallsTest {
public void canCaptureInboundFlowControlForServerStreamingObserver()
throws Exception {
ClientResponseObserver<Integer, String> responseObserver =
new ClientResponseObserver<Integer, String>() {
@Override
public void beforeStart(ClientCallStreamObserver<Integer> requestStream) {
requestStream.disableAutoInboundFlowControl();
requestStream.request(5);
}
class ResponseObserver implements ClientResponseObserver<Integer, String> {
@Override
public void onNext(String value) {
}
private ClientCallStreamObserver<Integer> requestStream;
@Override
public void onError(Throwable t) {
}
@Override
public void beforeStart(ClientCallStreamObserver<Integer> requestStream) {
this.requestStream = requestStream;
requestStream.disableAutoRequestWithInitial(0);
}
@Override
public void onCompleted() {
}
};
@Override
public void onNext(String value) {
}
@Override
public void onError(Throwable t) {
}
@Override
public void onCompleted() {
}
void request(int numMessages) {
requestStream.request(numMessages);
}
}
ResponseObserver responseObserver = new ResponseObserver();
final AtomicReference<ClientCall.Listener<String>> listener =
new AtomicReference<>();
final List<Integer> requests = new ArrayList<>();
@ -495,8 +545,9 @@ public class ClientCallsTest {
}
};
ClientCalls.asyncServerStreamingCall(call, 1, responseObserver);
responseObserver.request(5);
listener.get().onMessage("message");
assertThat(requests).containsExactly(5, 1).inOrder();
assertThat(requests).containsExactly(5).inOrder();
}
@Test
@ -544,7 +595,7 @@ public class ClientCallsTest {
new ClientResponseObserver<Integer, Integer>() {
@Override
public void beforeStart(final ClientCallStreamObserver<Integer> requestStream) {
requestStream.disableAutoInboundFlowControl();
requestStream.disableAutoRequestWithInitial(0);
}
@Override
@ -566,14 +617,15 @@ public class ClientCallsTest {
CallStreamObserver<Integer> integerStreamObserver = (CallStreamObserver<Integer>)
ClientCalls.asyncBidiStreamingCall(clientCall, responseObserver);
semaphore.acquire();
integerStreamObserver.request(1);
assertTrue(semaphore.tryAcquire(5, TimeUnit.SECONDS));
integerStreamObserver.request(2);
semaphore.acquire();
assertTrue(semaphore.tryAcquire(5, TimeUnit.SECONDS));
integerStreamObserver.request(3);
integerStreamObserver.onCompleted();
assertTrue(latch.await(5, TimeUnit.SECONDS));
// Verify that number of messages produced in each onReady handler call matches the number
// requested by the client. Note that ClientCalls.asyncBidiStreamingCall will request(1)
// requested by the client.
assertEquals(Arrays.asList(0, 1, 1, 2, 2, 2), receivedMessages);
}
@ -591,7 +643,7 @@ public class ClientCallsTest {
public StreamObserver<Integer> invoke(StreamObserver<Integer> responseObserver) {
final ServerCallStreamObserver<Integer> serverCallObserver =
(ServerCallStreamObserver<Integer>) responseObserver;
serverCallObserver.disableAutoInboundFlowControl();
serverCallObserver.disableAutoRequestWithInitial(0);
observerFuture.set(serverCallObserver);
return new StreamObserver<Integer>() {
@Override

View File

@ -260,7 +260,7 @@ public class ServerCallsTest {
}
@Test
public void cannotDisableAutoFlowControlAfterServiceInvocation() throws Exception {
public void cannotDisableAutoRequestAfterServiceInvocation() throws Exception {
final AtomicReference<ServerCallStreamObserver<Integer>> callObserver =
new AtomicReference<>();
ServerCallHandler<Integer, Integer> callHandler =
@ -276,7 +276,7 @@ public class ServerCallsTest {
callHandler.startCall(serverCall, new Metadata());
callListener.onMessage(1);
try {
callObserver.get().disableAutoInboundFlowControl();
callObserver.get().disableAutoRequestWithInitial(0);
fail("Cannot set onCancel handler after service invocation");
} catch (IllegalStateException expected) {
// Expected
@ -307,7 +307,30 @@ public class ServerCallsTest {
}
@Test
public void disablingInboundAutoFlowControlForUnaryHasNoEffect() throws Exception {
public void disablingInboundAutoRequestSuppressesRequestsForMoreMessages() throws Exception {
ServerCallHandler<Integer, Integer> callHandler =
ServerCalls.asyncBidiStreamingCall(
new ServerCalls.BidiStreamingMethod<Integer, Integer>() {
@Override
public StreamObserver<Integer> invoke(StreamObserver<Integer> responseObserver) {
ServerCallStreamObserver<Integer> serverCallObserver =
(ServerCallStreamObserver<Integer>) responseObserver;
serverCallObserver.disableAutoRequestWithInitial(0);
return new ServerCalls.NoopStreamObserver<>();
}
});
ServerCall.Listener<Integer> callListener =
callHandler.startCall(serverCall, new Metadata());
callListener.onReady();
// Transport should not call this if nothing has been requested but forcing it here
// to verify that message delivery does not trigger a call to request(1).
callListener.onMessage(1);
// Should never be called
assertThat(serverCall.requestCalls).isEmpty();
}
@Test
public void disablingInboundAutoRequestForUnaryHasNoEffect() throws Exception {
ServerCallHandler<Integer, Integer> callHandler =
ServerCalls.asyncUnaryCall(
new ServerCalls.UnaryMethod<Integer, Integer>() {
@ -315,7 +338,7 @@ public class ServerCallsTest {
public void invoke(Integer req, StreamObserver<Integer> responseObserver) {
ServerCallStreamObserver<Integer> serverCallObserver =
(ServerCallStreamObserver<Integer>) responseObserver;
serverCallObserver.disableAutoInboundFlowControl();
serverCallObserver.disableAutoRequestWithInitial(0);
}
});
callHandler.startCall(serverCall, new Metadata());