From 89f1dd617ae2613a1352a553fc3c6d134a3c62be Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Wed, 22 Aug 2018 08:30:21 -0700 Subject: [PATCH 1/5] Spanner: Block nested transactions Cloud spanner does not support nested transactions. Use a thread-local flag to check and throw exception. Signed-off-by: Nithin Nayak Sujir --- .../google-cloud-spanner/pom.xml | 2 + .../com/google/cloud/spanner/SpannerImpl.java | 20 +++- .../spanner/TransactionRunnerImplTest.java | 1 - .../cloud/spanner/it/ITTransactionTest.java | 110 ++++++++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/pom.xml b/google-cloud-clients/google-cloud-spanner/pom.xml index b57eb15bfdb2..f22dde4b3fa9 100644 --- a/google-cloud-clients/google-cloud-spanner/pom.xml +++ b/google-cloud-clients/google-cloud-spanner/pom.xml @@ -18,6 +18,7 @@ google-cloud-spanner + ${skipTests} @@ -27,6 +28,7 @@ 2.12.4 com.google.cloud.spanner.IntegrationTest + ${skipUnitTests} diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 850bf2898f57..f17c97dbcf45 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -81,7 +81,6 @@ import io.opencensus.trace.Span; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; - import java.io.IOException; import java.io.Serializable; import java.util.AbstractList; @@ -129,6 +128,19 @@ class SpannerImpl extends BaseService implements Spanner { private static final String QUERY = "CloudSpannerOperation.ExecuteStreamingQuery"; private static final String READ = "CloudSpannerOperation.ExecuteStreamingRead"; + private static final ThreadLocal hasPendingTransaction = new ThreadLocal() { + @Override + protected Boolean initialValue() { + return false; + } + }; + + private static void throwIfTransactionsPending() { + if (hasPendingTransaction.get() == Boolean.TRUE) { + throw newSpannerException(ErrorCode.INTERNAL, "Nested transactions are not supported"); + } + } + static { TraceUtil.exportSpans(CREATE_SESSION, DELETE_SESSION, BEGIN_TRANSACTION, COMMIT, QUERY, READ); } @@ -905,6 +917,8 @@ TransactionContextImpl newTransaction() { } T setActive(@Nullable T ctx) { + throwIfTransactionsPending(); + if (activeTransaction != null) { activeTransaction.invalidate(); } @@ -1239,11 +1253,13 @@ void backoffSleep(Context context, long backoffMillis) { @Override public T run(TransactionCallable callable) { try (Scope s = tracer.withSpan(span)) { + hasPendingTransaction.set(Boolean.TRUE); return runInternal(callable); } catch (RuntimeException e) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { + hasPendingTransaction.set(Boolean.FALSE); span.end(); } } @@ -1660,6 +1676,8 @@ ByteString getTransactionId() { } void initTransaction() { + throwIfTransactionsPending(); + // Since we only support synchronous calls, just block on "txnLock" while the RPC is in // flight. Note that we use the strategy of sending an explicit BeginTransaction() RPC, // rather than using the first read in the transaction to begin it implicitly. The chosen diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java index 9362814146ab..0cd804c6a89e 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java @@ -32,7 +32,6 @@ import io.grpc.Status; import io.grpc.StatusRuntimeException; import java.util.concurrent.atomic.AtomicInteger; - import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java index 1bacea50d627..139c75f66a04 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java @@ -23,13 +23,17 @@ import com.google.cloud.Timestamp; import com.google.cloud.spanner.AbortedException; +import com.google.cloud.spanner.BatchClient; +import com.google.cloud.spanner.BatchReadOnlyTransaction; import com.google.cloud.spanner.Database; import com.google.cloud.spanner.DatabaseClient; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.IntegrationTest; import com.google.cloud.spanner.IntegrationTestEnv; import com.google.cloud.spanner.Key; +import com.google.cloud.spanner.KeySet; import com.google.cloud.spanner.Mutation; +import com.google.cloud.spanner.PartitionOptions; import com.google.cloud.spanner.ReadContext; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; @@ -349,4 +353,110 @@ public Void run(TransactionContext transaction) throws SpannerException { .getLong(0)) .isEqualTo(2); } + + private void doNestedRwTransaction() { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws Exception { + return null; + } + }); + + return null; + } + }); + } + + @Test + public void nestedRwRwTransactionShouldThrowException() { + try { + doNestedRwTransaction(); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwRdTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client + .readOnlyTransaction() + .getReadTimestamp(); + + return null; + } + }); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwBatchTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + BatchClient batchClient = env.getTestHelper().getBatchClient(db); + BatchReadOnlyTransaction batchTxn = batchClient + .batchReadOnlyTransaction(TimestampBound.strong()); + batchTxn.partitionReadUsingIndex( + PartitionOptions.getDefaultInstance(), + "Test", + "Index", + KeySet.all(), + Arrays.asList("Fingerprint")); + + return null; + } + }); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwSingleUseReadTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client.singleUseReadOnlyTransaction(); + + return null; + } + }); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } } From 1062734fc4032993eaa21ed9c073b81c1b48ab81 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Fri, 14 Sep 2018 09:03:25 -0700 Subject: [PATCH 2/5] Add a flag to override default behaviour of nested transaction blocking --- .../com/google/cloud/spanner/SessionPool.java | 6 ++++++ .../com/google/cloud/spanner/SpannerImpl.java | 13 +++++++++++-- .../google/cloud/spanner/TransactionRunner.java | 2 ++ .../cloud/spanner/it/ITTransactionTest.java | 16 ++++++++++++++++ 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java index 80e4d67eb664..ced479151253 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java @@ -408,6 +408,12 @@ public T run(TransactionCallable callable) { public Timestamp getCommitTimestamp() { return runner.getCommitTimestamp(); } + + @Override + public TransactionRunner allowNestedTransaction() { + runner.allowNestedTransaction(); + return runner; + } }; } diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index f17c97dbcf45..77fbd064fa88 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -1236,6 +1236,11 @@ void backoffSleep(Context context, long backoffMillis) { private final Span span; private TransactionContextImpl txn; private volatile boolean isValid = true; + private boolean blockNestedTxn = true; + public TransactionRunner allowNestedTransaction() { + blockNestedTxn = false; + return this; + } TransactionRunnerImpl( SessionImpl session, SpannerRpc rpc, Sleeper sleeper, int defaultPrefetchChunks) { @@ -1253,13 +1258,17 @@ void backoffSleep(Context context, long backoffMillis) { @Override public T run(TransactionCallable callable) { try (Scope s = tracer.withSpan(span)) { - hasPendingTransaction.set(Boolean.TRUE); + if (blockNestedTxn) + hasPendingTransaction.set(Boolean.TRUE); + return runInternal(callable); } catch (RuntimeException e) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { - hasPendingTransaction.set(Boolean.FALSE); + if (blockNestedTxn) + hasPendingTransaction.set(Boolean.FALSE); + span.end(); } } diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java index 65659ec98b7e..eacdeb30a04b 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java @@ -73,4 +73,6 @@ interface TransactionCallable { * {@link #run(TransactionCallable)} has returned normally. */ Timestamp getCommitTimestamp(); + + TransactionRunner allowNestedTransaction(); } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java index 139c75f66a04..dfc218c4f4c2 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java @@ -459,4 +459,20 @@ public Void run(TransactionContext transaction) throws SpannerException { assertThat(e.getMessage()).contains("not supported"); } } + + @Test + public void nestedTxnShouldSucceedWhenAllowed() { + client + .readWriteTransaction() + .allowNestedTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client.singleUseReadOnlyTransaction(); + + return null; + } + }); + } } From 604e0622c8f09178b19608830f05f08eeca1ff17 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Fri, 14 Sep 2018 09:46:12 -0700 Subject: [PATCH 3/5] Fix codacy warnings --- .../main/java/com/google/cloud/spanner/SpannerImpl.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 77fbd064fa88..3642f1df32ce 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -1223,6 +1223,7 @@ public void execute(Runnable command) { @VisibleForTesting static class TransactionRunnerImpl implements SessionTransaction, TransactionRunner { + private boolean blockNestedTxn = true; /** Allow for testing of backoff logic */ static class Sleeper { @@ -1236,7 +1237,7 @@ void backoffSleep(Context context, long backoffMillis) { private final Span span; private TransactionContextImpl txn; private volatile boolean isValid = true; - private boolean blockNestedTxn = true; + public TransactionRunner allowNestedTransaction() { blockNestedTxn = false; return this; @@ -1258,16 +1259,18 @@ public TransactionRunner allowNestedTransaction() { @Override public T run(TransactionCallable callable) { try (Scope s = tracer.withSpan(span)) { - if (blockNestedTxn) + if (blockNestedTxn) { hasPendingTransaction.set(Boolean.TRUE); + } return runInternal(callable); } catch (RuntimeException e) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { - if (blockNestedTxn) + if (blockNestedTxn) { hasPendingTransaction.set(Boolean.FALSE); + } span.end(); } From 72409fddaf5ce63b63c2a392e50439b55a92ab33 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Fri, 14 Sep 2018 11:25:39 -0700 Subject: [PATCH 4/5] Add detailed comment for allowNestedTransaction(), make test names clearer --- .../google/cloud/spanner/TransactionRunner.java | 17 +++++++++++++++++ .../cloud/spanner/it/ITTransactionTest.java | 10 +++++----- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java index eacdeb30a04b..02d5947109d7 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunner.java @@ -74,5 +74,22 @@ interface TransactionCallable { */ Timestamp getCommitTimestamp(); + /** + * Allows overriding the default behaviour of blocking nested transactions. + * + * Note that the client library does not maintain any information regarding the nesting structure. + * If an outer transaction fails and an inner transaction succeeds, upon retry of the outer + * transaction, the inner transaction will be re-executed. + * + * Use with care when certain that the inner transaction is idempotent. Avoid doing this when + * accessing the same db. There might be legitimate uses where access need to be made across DBs + * for instance. + * + * E.g. of nesting that is discouraged, see {@code nestedReadWriteTxnThrows} + * {@code nestedReadOnlyTxnThrows}, {@code nestedBatchTxnThrows}, + * {@code nestedSingleUseReadTxnThrows} + * + * @return this object + */ TransactionRunner allowNestedTransaction(); } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java index dfc218c4f4c2..c9fb40416fef 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java @@ -377,7 +377,7 @@ public Void run(TransactionContext transaction) throws Exception { } @Test - public void nestedRwRwTransactionShouldThrowException() { + public void nestedReadWriteTxnThrows() { try { doNestedRwTransaction(); fail("Expected exception"); @@ -388,7 +388,7 @@ public void nestedRwRwTransactionShouldThrowException() { } @Test - public void nestedRwRdTransactionShouldThrowException() { + public void nestedReadOnlyTxnThrows() { try { client .readWriteTransaction() @@ -411,7 +411,7 @@ public Void run(TransactionContext transaction) throws SpannerException { } @Test - public void nestedRwBatchTransactionShouldThrowException() { + public void nestedBatchTxnThrows() { try { client .readWriteTransaction() @@ -440,7 +440,7 @@ public Void run(TransactionContext transaction) throws SpannerException { } @Test - public void nestedRwSingleUseReadTransactionShouldThrowException() { + public void nestedSingleUseReadTxnThrows() { try { client .readWriteTransaction() @@ -461,7 +461,7 @@ public Void run(TransactionContext transaction) throws SpannerException { } @Test - public void nestedTxnShouldSucceedWhenAllowed() { + public void nestedTxnSucceedsWhenAllowed() { client .readWriteTransaction() .allowNestedTransaction() From d2dd3dce0c07f9dd8a8c1ce6c1346be424eb79f8 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Mon, 17 Sep 2018 14:34:11 -0700 Subject: [PATCH 5/5] Remove hasPendingTransaction unconditionally --- .../main/java/com/google/cloud/spanner/SpannerImpl.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 3642f1df32ce..579da6797ced 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -1268,10 +1268,10 @@ public T run(TransactionCallable callable) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { - if (blockNestedTxn) { - hasPendingTransaction.set(Boolean.FALSE); - } - + // Remove threadLocal rather than set to FALSE to avoid a possible memory leak. + // We also do this unconditionally in case a user has modified the flag when the transaction + // was running. + hasPendingTransaction.remove(); span.end(); } }