From 84969fbdad0674b5db3d9e0215225a4f35b0854e Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Fri, 8 Mar 2019 17:20:30 +0100 Subject: [PATCH 1/3] #3889 throw exception when an SSLHandshakeException occurs SSLHandshakeExceptions are not retryable, as it is most probably an indication that the client does not accept the server certificate. --- .../spanner/SpannerExceptionFactory.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java index 3ff2d6749778..0c90f05aae04 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java @@ -16,18 +16,18 @@ package com.google.cloud.spanner; -import static com.google.cloud.spanner.SpannerException.DoNotConstructDirectly; - +import java.util.concurrent.CancellationException; +import java.util.concurrent.TimeoutException; +import javax.annotation.Nullable; +import javax.net.ssl.SSLHandshakeException; import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.rpc.ApiException; +import com.google.cloud.spanner.SpannerException.DoNotConstructDirectly; import com.google.common.base.MoreObjects; import com.google.common.base.Predicate; import io.grpc.Context; import io.grpc.Status; import io.grpc.StatusRuntimeException; -import java.util.concurrent.CancellationException; -import java.util.concurrent.TimeoutException; -import javax.annotation.Nullable; /** * A factory for creating instances of {@link SpannerException} and its subtypes. All creation of @@ -168,7 +168,9 @@ private static boolean isRetryable(ErrorCode code, @Nullable Throwable cause) { case INTERNAL: return hasCauseMatching(cause, Matchers.isRetryableInternalError); case UNAVAILABLE: - return true; + // SSLHandshakeException is (probably) not retryable, as it is an indication that the server + // certificate was not accepted by the client. + return !hasCauseMatching(cause, Matchers.isSSLHandshakeException); case RESOURCE_EXHAUSTED: return SpannerException.extractRetryDelay(cause) > 0; default: @@ -211,5 +213,11 @@ public boolean apply(Throwable cause) { return false; } }; + static final Predicate isSSLHandshakeException = new Predicate() { + @Override + public boolean apply(Throwable input) { + return input instanceof SSLHandshakeException; + } + }; } } From da72d8a4f7d05a86ed9420df7bafc681f8a498ff Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Fri, 8 Mar 2019 17:43:50 +0100 Subject: [PATCH 2/3] #3889 added test for retryability of SSLHandshakeException --- .../google/cloud/spanner/SpannerImplTest.java | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java index 8dffda872401..10943700e613 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java @@ -17,13 +17,13 @@ package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; - -import com.google.cloud.grpc.GrpcTransportOptions; -import com.google.cloud.spanner.spi.v1.SpannerRpc; import java.util.HashMap; import java.util.Map; import java.util.concurrent.Callable; +import javax.net.ssl.SSLHandshakeException; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,6 +33,8 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import com.google.cloud.grpc.GrpcTransportOptions; +import com.google.cloud.spanner.spi.v1.SpannerRpc; /** Unit tests for {@link SpannerImpl}. */ @RunWith(JUnit4.class) @@ -133,4 +135,44 @@ public Void call() throws Exception { assertThat(e.getMessage().contains("Unexpected exception thrown")); } } + + @Test + public void sslHandshakeExceptionIsNotRetryable() { + // Verify that a SpannerException with code UNAVAILABLE and cause SSLHandshakeException is not + // retryable. + boolean gotExpectedException = false; + try { + SpannerImpl.runWithRetries(new Callable() { + @Override + public Void call() throws Exception { + throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, + "This exception should not be retryable", + new SSLHandshakeException("some SSL handshake exception")); + } + }); + } catch (SpannerException e) { + gotExpectedException = true; + assertThat(e.isRetryable(), is(false)); + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.UNAVAILABLE); + assertThat(e.getMessage().contains("This exception should not be retryable")); + } + assertThat(gotExpectedException, is(true)); + + // Verify that any other SpannerException with code UNAVAILABLE is retryable. + SpannerImpl.runWithRetries(new Callable() { + private boolean firstTime = true; + + @Override + public Void call() throws Exception { + // Keep track of whethr this is the first call or a subsequent call to avoid an infinite + // loop. + if (firstTime) { + firstTime = false; + throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, + "This exception should be retryable", new Exception("some other exception")); + } + return null; + } + }); + } } From 3f6dd4888ea9f552127efcfd93790c9d4574b219 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Fri, 8 Mar 2019 19:16:57 +0100 Subject: [PATCH 3/3] fixed formatting --- .../spanner/SpannerExceptionFactory.java | 21 +++---- .../google/cloud/spanner/SpannerImplTest.java | 56 ++++++++++--------- 2 files changed, 42 insertions(+), 35 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java index 0c90f05aae04..7577a06bfbce 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java @@ -16,10 +16,6 @@ package com.google.cloud.spanner; -import java.util.concurrent.CancellationException; -import java.util.concurrent.TimeoutException; -import javax.annotation.Nullable; -import javax.net.ssl.SSLHandshakeException; import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.rpc.ApiException; import com.google.cloud.spanner.SpannerException.DoNotConstructDirectly; @@ -28,6 +24,10 @@ import io.grpc.Context; import io.grpc.Status; import io.grpc.StatusRuntimeException; +import java.util.concurrent.CancellationException; +import java.util.concurrent.TimeoutException; +import javax.annotation.Nullable; +import javax.net.ssl.SSLHandshakeException; /** * A factory for creating instances of {@link SpannerException} and its subtypes. All creation of @@ -213,11 +213,12 @@ public boolean apply(Throwable cause) { return false; } }; - static final Predicate isSSLHandshakeException = new Predicate() { - @Override - public boolean apply(Throwable input) { - return input instanceof SSLHandshakeException; - } - }; + static final Predicate isSSLHandshakeException = + new Predicate() { + @Override + public boolean apply(Throwable input) { + return input instanceof SSLHandshakeException; + } + }; } } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java index 10943700e613..93c20dfb19b3 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java @@ -20,6 +20,9 @@ import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; + +import com.google.cloud.grpc.GrpcTransportOptions; +import com.google.cloud.spanner.spi.v1.SpannerRpc; import java.util.HashMap; import java.util.Map; import java.util.concurrent.Callable; @@ -33,8 +36,6 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; -import com.google.cloud.grpc.GrpcTransportOptions; -import com.google.cloud.spanner.spi.v1.SpannerRpc; /** Unit tests for {@link SpannerImpl}. */ @RunWith(JUnit4.class) @@ -142,14 +143,16 @@ public void sslHandshakeExceptionIsNotRetryable() { // retryable. boolean gotExpectedException = false; try { - SpannerImpl.runWithRetries(new Callable() { - @Override - public Void call() throws Exception { - throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, - "This exception should not be retryable", - new SSLHandshakeException("some SSL handshake exception")); - } - }); + SpannerImpl.runWithRetries( + new Callable() { + @Override + public Void call() throws Exception { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.UNAVAILABLE, + "This exception should not be retryable", + new SSLHandshakeException("some SSL handshake exception")); + } + }); } catch (SpannerException e) { gotExpectedException = true; assertThat(e.isRetryable(), is(false)); @@ -159,20 +162,23 @@ public Void call() throws Exception { assertThat(gotExpectedException, is(true)); // Verify that any other SpannerException with code UNAVAILABLE is retryable. - SpannerImpl.runWithRetries(new Callable() { - private boolean firstTime = true; - - @Override - public Void call() throws Exception { - // Keep track of whethr this is the first call or a subsequent call to avoid an infinite - // loop. - if (firstTime) { - firstTime = false; - throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, - "This exception should be retryable", new Exception("some other exception")); - } - return null; - } - }); + SpannerImpl.runWithRetries( + new Callable() { + private boolean firstTime = true; + + @Override + public Void call() throws Exception { + // Keep track of whethr this is the first call or a subsequent call to avoid an infinite + // loop. + if (firstTime) { + firstTime = false; + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.UNAVAILABLE, + "This exception should be retryable", + new Exception("some other exception")); + } + return null; + } + }); } }