From 54070043bc1b8a10a3819c1b63a9f736a3cffae9 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 10 Nov 2022 16:31:02 -0500 Subject: [PATCH 1/7] fix: Use non-eager FutureListener for httpjson --- .../api/gax/httpjson/HttpJsonClientCalls.java | 38 +++++++++++++------ .../gax/httpjson/HttpJsonDirectCallable.java | 2 +- .../HttpJsonClientInterceptorTest.java | 18 +-------- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java index 9512537cd..6376e4f94 100644 --- a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java +++ b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java @@ -64,7 +64,7 @@ public static HttpJsonClientCall newC return httpJsonContext.getChannel().newCall(methodDescriptor, httpJsonContext.getCallOptions()); } - static ApiFuture eagerFutureUnaryCall( + static ApiFuture futureUnaryCall( HttpJsonClientCall clientCall, RequestT request) { // Start the call HttpJsonFuture future = new HttpJsonFuture<>(clientCall); @@ -115,34 +115,48 @@ public boolean setException(Throwable throwable) { private static class FutureListener extends HttpJsonClientCall.Listener { private final HttpJsonFuture future; + private T message; + private boolean isMessageReceived; private FutureListener(HttpJsonFuture future) { this.future = future; + this.isMessageReceived = false; } @Override public void onMessage(T message) { - if (!future.set(message)) { + if (isMessageReceived) { throw new IllegalStateException("More than one value received for unary call"); } + this.message = message; + this.isMessageReceived = true; } @Override public void onClose(int statusCode, HttpJsonMetadata trailers) { - if (!future.isDone()) { - if (trailers == null || trailers.getException() == null) { - future.setException( - new HttpJsonStatusRuntimeException( - statusCode, - "Exception during a client call closure", - new NullPointerException( - "Both response message and response exception were null"))); + if (statusCode >= 200 && statusCode < 400) { + if (!isMessageReceived) { + if (trailers == null || trailers.getException() == null) { + future.setException( + new HttpJsonStatusRuntimeException( + statusCode, + "Exception during a client call closure", + new NullPointerException( + "Both response message and response exception were null"))); + } else { + future.setException( + new HttpJsonStatusRuntimeException( + statusCode, + "Exception during a client call closure", + new RuntimeException("No message received for unary call"))); + } } else { - future.setException(trailers.getException()); + future.set(message); } - } else if (statusCode < 200 || statusCode >= 400) { + } else { LOGGER.log( Level.WARNING, "Received error for unary call after receiving a successful response"); + future.setException(trailers.getException()); } } } diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectCallable.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectCallable.java index dd0826dd6..c631ca60c 100644 --- a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectCallable.java +++ b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectCallable.java @@ -65,7 +65,7 @@ public ApiFuture futureCall(RequestT request, ApiCallContext inputCon HttpJsonClientCall clientCall = HttpJsonClientCalls.newCall(descriptor, context); - return HttpJsonClientCalls.eagerFutureUnaryCall(clientCall, request); + return HttpJsonClientCalls.futureUnaryCall(clientCall, request); } @Override diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonClientInterceptorTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonClientInterceptorTest.java index 9a27be86f..6e081fb75 100644 --- a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonClientInterceptorTest.java +++ b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonClientInterceptorTest.java @@ -56,9 +56,6 @@ @RunWith(JUnit4.class) public class HttpJsonClientInterceptorTest { - private static final int NUM_RETRIES = 10; - private static final int BUSY_WAIT_TIME_IN_MS = 1000; - private static class CapturingClientInterceptor implements HttpJsonClientInterceptor { // Manually capturing arguments instead of using Mockito. This is intentional, as this // specific test interceptor class represents a typical interceptor implementation. Doing the @@ -213,21 +210,8 @@ public void testCustomInterceptor() throws ExecutionException, InterruptedExcept // Test that internal interceptor worked (the one which inserts headers) assertThat(headerValue).isEqualTo("headerValue"); - // Test that the custom interceptor was called - // These two tests cases are guaranteed to respond back by the time `get()` is unblocked + assertThat(interceptor.capturedStatusCode).isEqualTo(200); assertThat(interceptor.capturedResponseHeaders).isNotNull(); assertThat(interceptor.capturedMessage).isEqualTo(request); - - // Attempt to busy wait {NUM_RETRIES} times until the interceptor's `onClose()` is called - int attempts = 0; - while (interceptor.capturedStatusCode == 0) { - Thread.sleep(BUSY_WAIT_TIME_IN_MS); - attempts++; - if (attempts == NUM_RETRIES) { - break; - } - } - - assertThat(interceptor.capturedStatusCode).isEqualTo(200); } } From a082acce5a1a51be532a944427e056cb8a1c23c8 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Mon, 14 Nov 2022 14:02:42 -0500 Subject: [PATCH 2/7] chore: Fix onClose logic --- .../api/gax/httpjson/HttpJsonClientCalls.java | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java index 6376e4f94..96d29f49f 100644 --- a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java +++ b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java @@ -134,29 +134,22 @@ public void onMessage(T message) { @Override public void onClose(int statusCode, HttpJsonMetadata trailers) { - if (statusCode >= 200 && statusCode < 400) { - if (!isMessageReceived) { - if (trailers == null || trailers.getException() == null) { - future.setException( - new HttpJsonStatusRuntimeException( - statusCode, - "Exception during a client call closure", - new NullPointerException( - "Both response message and response exception were null"))); - } else { - future.setException( - new HttpJsonStatusRuntimeException( - statusCode, - "Exception during a client call closure", - new RuntimeException("No message received for unary call"))); - } + if (!isMessageReceived) { + if (trailers == null || trailers.getException() == null) { + future.setException( + new HttpJsonStatusRuntimeException( + statusCode, + "Exception during a client call closure", + new NullPointerException( + "Both response message and response exception were null"))); } else { - future.set(message); + future.setException(trailers.getException()); } - } else { + } else if (statusCode < 200 || statusCode >= 400) { LOGGER.log( Level.WARNING, "Received error for unary call after receiving a successful response"); - future.setException(trailers.getException()); + } else { + future.set(message); } } } From 4442eb10c4fc323778a58979756db7b896700daf Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 15 Dec 2022 12:01:09 -0500 Subject: [PATCH 3/7] chore: Check if storedMessage is null --- .../api/gax/httpjson/HttpJsonClientCalls.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java index 96d29f49f..4f55aa0dd 100644 --- a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java +++ b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java @@ -115,26 +115,23 @@ public boolean setException(Throwable throwable) { private static class FutureListener extends HttpJsonClientCall.Listener { private final HttpJsonFuture future; - private T message; - private boolean isMessageReceived; + private T storedMessage; private FutureListener(HttpJsonFuture future) { this.future = future; - this.isMessageReceived = false; } @Override public void onMessage(T message) { - if (isMessageReceived) { + if (storedMessage != null) { throw new IllegalStateException("More than one value received for unary call"); } - this.message = message; - this.isMessageReceived = true; + storedMessage = message; } @Override public void onClose(int statusCode, HttpJsonMetadata trailers) { - if (!isMessageReceived) { + if (storedMessage == null) { if (trailers == null || trailers.getException() == null) { future.setException( new HttpJsonStatusRuntimeException( @@ -149,7 +146,7 @@ public void onClose(int statusCode, HttpJsonMetadata trailers) { LOGGER.log( Level.WARNING, "Received error for unary call after receiving a successful response"); } else { - future.set(message); + future.set(storedMessage); } } } From 972ec87d7351abc018ab2782e035dead8ee1ea91 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 15 Dec 2022 16:47:53 -0500 Subject: [PATCH 4/7] chore: Add test cases for non eager listener --- .../httpjson/HttpJsonDirectCallableTest.java | 116 +++++++++++++++++- 1 file changed, 110 insertions(+), 6 deletions(-) diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java index e47719c5c..582c47960 100644 --- a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java +++ b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java @@ -139,7 +139,7 @@ public void testSuccessfulUnaryResponse() throws ExecutionException, Interrupted Field request; Field expectedResponse; - request = expectedResponse = createTestMessage(); + request = expectedResponse = createTestMessage(2); MOCK_SERVICE.addResponse(expectedResponse); @@ -151,6 +151,73 @@ public void testSuccessfulUnaryResponse() throws ExecutionException, Interrupted assertThat(headerValue).isEqualTo("headerValue"); } + /** + * This test is for a Unary Call where server mistakenly sends multiple responses back + * The expectation for this MOCK_SERVICE is to return what was sent into the request + * i.e. callable.futureCall(x) -> x + *

+ * For a Unary Call, gax will return only the first (and hopefully only) response back. + * @throws InterruptedException + * @throws ExecutionException + */ + @Test + public void testSuccessfulMultipleResponsesForUnaryCall() throws InterruptedException, ExecutionException { + HttpJsonDirectCallable callable = + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + + HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); + + Field request; + Field expectedResponse; + request = expectedResponse = createTestMessage(2); + Field otherResponse = createTestMessage(10); + MOCK_SERVICE.addResponse(expectedResponse); + MOCK_SERVICE.addResponse(otherResponse); + MOCK_SERVICE.addResponse(otherResponse); + + Field actualResponse = callable.futureCall(request, callContext).get(); + assertThat(actualResponse).isEqualTo(expectedResponse); + assertThat(MOCK_SERVICE.getRequestPaths().size()).isEqualTo(1); + String headerValue = MOCK_SERVICE.getRequestHeaders().get("header-key").iterator().next(); + assertThat(headerValue).isEqualTo("headerValue"); + } + + /** + * This test is for a Unary Call where server mistakenly sends multiple responses back + * The expectation for this MOCK_SERVICE is to return what was sent into the request + * i.e. callable.futureCall(x) -> x + *

+ * For a Unary Call, gax will return only the first (and hopefully only) response back. + * @throws InterruptedException + * @throws ExecutionException + */ + @Test + public void testErrorMultipleResponsesForUnaryCall() throws InterruptedException, ExecutionException { + HttpJsonDirectCallable callable = + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + + HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); + + Field request; + Field expectedResponse; + request = expectedResponse = createTestMessage(2); + Field randomResponse1 = createTestMessage(10); + Field randomResponse2 = createTestMessage(3); + MOCK_SERVICE.addResponse(randomResponse1); + MOCK_SERVICE.addResponse(expectedResponse); + MOCK_SERVICE.addResponse(randomResponse2); + + Field actualResponse = callable.futureCall(request, callContext).get(); + assertThat(actualResponse).isNotEqualTo(expectedResponse); + assertThat(MOCK_SERVICE.getRequestPaths().size()).isEqualTo(1); + String headerValue = MOCK_SERVICE.getRequestHeaders().get("header-key").iterator().next(); + assertThat(headerValue).isEqualTo("headerValue"); + } + + /** + * The expectation for gax is that an exception from the server will return an exception response + * @throws InterruptedException + */ @Test public void testErrorUnaryResponse() throws InterruptedException { HttpJsonDirectCallable callable = @@ -164,7 +231,7 @@ public void testErrorUnaryResponse() throws InterruptedException { MOCK_SERVICE.addException(exception); try { - callable.futureCall(createTestMessage(), callContext).get(); + callable.futureCall(createTestMessage(2), callContext).get(); Assert.fail("No exception raised"); } catch (ExecutionException e) { HttpResponseException respExp = (HttpResponseException) e.getCause(); @@ -173,6 +240,12 @@ public void testErrorUnaryResponse() throws InterruptedException { } } + /** + * This test is for a Unary Call where server sends back a null value but successful status code + * Gax expects the response back to be parse-able into JSON but the null value is not valid. + * This will throw an Exception for a successful response but invalid content + * @throws InterruptedException + */ @Test public void testErrorNullContentSuccessfulResponse() throws InterruptedException { HttpJsonDirectCallable callable = @@ -183,7 +256,7 @@ public void testErrorNullContentSuccessfulResponse() throws InterruptedException MOCK_SERVICE.addNullResponse(); try { - callable.futureCall(createTestMessage(), callContext).get(); + callable.futureCall(createTestMessage(2), callContext).get(); Assert.fail("No exception raised"); } catch (ExecutionException e) { HttpJsonStatusRuntimeException respExp = (HttpJsonStatusRuntimeException) e.getCause(); @@ -193,6 +266,11 @@ public void testErrorNullContentSuccessfulResponse() throws InterruptedException } } + /** + * The expectation for an exception from the server is an exception response + * regardless of what content is sent back + * @throws InterruptedException + */ @Test public void testErrorNullContentFailedResponse() throws InterruptedException { HttpJsonDirectCallable callable = @@ -202,7 +280,7 @@ public void testErrorNullContentFailedResponse() throws InterruptedException { MOCK_SERVICE.addNullResponse(400); try { - callable.futureCall(createTestMessage(), callContext).get(); + callable.futureCall(createTestMessage(2), callContext).get(); Assert.fail("No exception raised"); } catch (ExecutionException e) { HttpResponseException respExp = (HttpResponseException) e.getCause(); @@ -211,10 +289,36 @@ public void testErrorNullContentFailedResponse() throws InterruptedException { } } - private Field createTestMessage() { + /** + * Expectation is that an Exception is returned even on a non 4xx status code + * @throws InterruptedException + */ + @Test + public void testErrorNon2xxOr4xxResponse() throws InterruptedException { + HttpJsonDirectCallable callable = + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + + HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); + + ApiException exception = + ApiExceptionFactory.createException( + new Exception(), FakeStatusCode.of(Code.INTERNAL), false); + MOCK_SERVICE.addException(500, exception); + + try { + callable.futureCall(createTestMessage(2), callContext).get(); + Assert.fail("No exception raised"); + } catch (ExecutionException e) { + HttpResponseException respExp = (HttpResponseException) e.getCause(); + assertThat(respExp.getStatusCode()).isEqualTo(500); + assertThat(respExp.getContent()).isEqualTo(exception.toString()); + } + } + + private Field createTestMessage(int number) { return Field.newBuilder() // "echo" service .setName("john/imTheBestField") - .setNumber(2) + .setNumber(number) .setCardinality(Cardinality.CARDINALITY_OPTIONAL) .setDefaultValue("blah") .build(); From f307b6ba6febc304ce8d3f9583071cdf98a93560 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 15 Dec 2022 16:50:30 -0500 Subject: [PATCH 5/7] chore: Fix tests --- .../com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java index 582c47960..3732c2c3b 100644 --- a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java +++ b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java @@ -208,6 +208,8 @@ public void testErrorMultipleResponsesForUnaryCall() throws InterruptedException MOCK_SERVICE.addResponse(randomResponse2); Field actualResponse = callable.futureCall(request, callContext).get(); + // Gax returns the first response for Unary Call + assertThat(actualResponse).isEqualTo(randomResponse1); assertThat(actualResponse).isNotEqualTo(expectedResponse); assertThat(MOCK_SERVICE.getRequestPaths().size()).isEqualTo(1); String headerValue = MOCK_SERVICE.getRequestHeaders().get("header-key").iterator().next(); From 8ff7feb8dd763dcdb1c76df05d580cfb5482a162 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 15 Dec 2022 16:56:43 -0500 Subject: [PATCH 6/7] chore: Fix formatting --- .../httpjson/HttpJsonDirectCallableTest.java | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java index 3732c2c3b..e0dbf26ff 100644 --- a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java +++ b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java @@ -152,18 +152,20 @@ public void testSuccessfulUnaryResponse() throws ExecutionException, Interrupted } /** - * This test is for a Unary Call where server mistakenly sends multiple responses back - * The expectation for this MOCK_SERVICE is to return what was sent into the request - * i.e. callable.futureCall(x) -> x - *

- * For a Unary Call, gax will return only the first (and hopefully only) response back. + * This test is for a Unary Call where server mistakenly sends multiple responses back The + * expectation for this MOCK_SERVICE is to return what was sent into the request i.e. + * callable.futureCall(x) -> x + * + *

For a Unary Call, gax will return only the first (and hopefully only) response back. + * * @throws InterruptedException * @throws ExecutionException */ @Test - public void testSuccessfulMultipleResponsesForUnaryCall() throws InterruptedException, ExecutionException { + public void testSuccessfulMultipleResponsesForUnaryCall() + throws InterruptedException, ExecutionException { HttpJsonDirectCallable callable = - new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); @@ -183,18 +185,20 @@ public void testSuccessfulMultipleResponsesForUnaryCall() throws InterruptedExce } /** - * This test is for a Unary Call where server mistakenly sends multiple responses back - * The expectation for this MOCK_SERVICE is to return what was sent into the request - * i.e. callable.futureCall(x) -> x - *

- * For a Unary Call, gax will return only the first (and hopefully only) response back. + * This test is for a Unary Call where server mistakenly sends multiple responses back The + * expectation for this MOCK_SERVICE is to return what was sent into the request i.e. + * callable.futureCall(x) -> x + * + *

For a Unary Call, gax will return only the first (and hopefully only) response back. + * * @throws InterruptedException * @throws ExecutionException */ @Test - public void testErrorMultipleResponsesForUnaryCall() throws InterruptedException, ExecutionException { + public void testErrorMultipleResponsesForUnaryCall() + throws InterruptedException, ExecutionException { HttpJsonDirectCallable callable = - new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); @@ -218,6 +222,7 @@ public void testErrorMultipleResponsesForUnaryCall() throws InterruptedException /** * The expectation for gax is that an exception from the server will return an exception response + * * @throws InterruptedException */ @Test @@ -244,8 +249,9 @@ public void testErrorUnaryResponse() throws InterruptedException { /** * This test is for a Unary Call where server sends back a null value but successful status code - * Gax expects the response back to be parse-able into JSON but the null value is not valid. - * This will throw an Exception for a successful response but invalid content + * Gax expects the response back to be parse-able into JSON but the null value is not valid. This + * will throw an Exception for a successful response but invalid content + * * @throws InterruptedException */ @Test @@ -269,8 +275,9 @@ public void testErrorNullContentSuccessfulResponse() throws InterruptedException } /** - * The expectation for an exception from the server is an exception response - * regardless of what content is sent back + * The expectation for an exception from the server is an exception response regardless of what + * content is sent back + * * @throws InterruptedException */ @Test @@ -293,18 +300,19 @@ public void testErrorNullContentFailedResponse() throws InterruptedException { /** * Expectation is that an Exception is returned even on a non 4xx status code + * * @throws InterruptedException */ @Test public void testErrorNon2xxOr4xxResponse() throws InterruptedException { HttpJsonDirectCallable callable = - new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); + new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR); HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); ApiException exception = - ApiExceptionFactory.createException( - new Exception(), FakeStatusCode.of(Code.INTERNAL), false); + ApiExceptionFactory.createException( + new Exception(), FakeStatusCode.of(Code.INTERNAL), false); MOCK_SERVICE.addException(500, exception); try { From dc0a6fd4ccc85cc53d1124db9eccc57636eacc58 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Fri, 16 Dec 2022 17:13:55 -0500 Subject: [PATCH 7/7] chore: Fix comments --- .../api/gax/httpjson/HttpJsonClientCalls.java | 16 ++++++++-------- .../gax/httpjson/HttpJsonDirectCallableTest.java | 16 +++++++--------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java index 4f55aa0dd..1b6ab41d7 100644 --- a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java +++ b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java @@ -115,23 +115,26 @@ public boolean setException(Throwable throwable) { private static class FutureListener extends HttpJsonClientCall.Listener { private final HttpJsonFuture future; - private T storedMessage; + private T message; + private boolean isMessageReceived; private FutureListener(HttpJsonFuture future) { this.future = future; + this.isMessageReceived = false; } @Override public void onMessage(T message) { - if (storedMessage != null) { + if (isMessageReceived) { throw new IllegalStateException("More than one value received for unary call"); } - storedMessage = message; + isMessageReceived = true; + this.message = message; } @Override public void onClose(int statusCode, HttpJsonMetadata trailers) { - if (storedMessage == null) { + if (!isMessageReceived) { if (trailers == null || trailers.getException() == null) { future.setException( new HttpJsonStatusRuntimeException( @@ -142,11 +145,8 @@ public void onClose(int statusCode, HttpJsonMetadata trailers) { } else { future.setException(trailers.getException()); } - } else if (statusCode < 200 || statusCode >= 400) { - LOGGER.log( - Level.WARNING, "Received error for unary call after receiving a successful response"); } else { - future.set(storedMessage); + future.set(message); } } } diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java index e0dbf26ff..2e9d5cb08 100644 --- a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java +++ b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java @@ -169,9 +169,8 @@ public void testSuccessfulMultipleResponsesForUnaryCall() HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); - Field request; - Field expectedResponse; - request = expectedResponse = createTestMessage(2); + Field request = createTestMessage(2); + Field expectedResponse = createTestMessage(2); Field otherResponse = createTestMessage(10); MOCK_SERVICE.addResponse(expectedResponse); MOCK_SERVICE.addResponse(otherResponse); @@ -202,9 +201,8 @@ public void testErrorMultipleResponsesForUnaryCall() HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel); - Field request; - Field expectedResponse; - request = expectedResponse = createTestMessage(2); + Field request = createTestMessage(2); + Field expectedResponse = createTestMessage(2); Field randomResponse1 = createTestMessage(10); Field randomResponse2 = createTestMessage(3); MOCK_SERVICE.addResponse(randomResponse1); @@ -275,8 +273,8 @@ public void testErrorNullContentSuccessfulResponse() throws InterruptedException } /** - * The expectation for an exception from the server is an exception response regardless of what - * content is sent back + * The expectation for a non-2xx from the server is an exception response regardless of the + * content sent back * * @throws InterruptedException */ @@ -299,7 +297,7 @@ public void testErrorNullContentFailedResponse() throws InterruptedException { } /** - * Expectation is that an Exception is returned even on a non 4xx status code + * Expectation is that an Exception is returned even on a non-2xx status code * * @throws InterruptedException */