Skip to content
This repository was archived by the owner on Apr 7, 2026. It is now read-only.

Commit 9500114

Browse files
olavloiteolavloite
committed
fix: mark transaction as invalid if no tx is returned before RS is closed
If a query requests the begin of a new transaction, the transaction id is returned by the first call to ResultSet#next(). If the ResultSet is closed by another thread before the first result has been returned, or before that result has been consumed internally to set the transaction id, no transaction id will be set. This will cause any subsequent statement in the same transaction to timeout while waiting for a transaction to be returned.
1 parent 3f28c46 commit 9500114

6 files changed

Lines changed: 93 additions & 8 deletions

File tree

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@ public void onTransactionMetadata(Transaction transaction) {}
691691
public void onError(SpannerException e, boolean withBeginTransaction) {}
692692

693693
@Override
694-
public void onDone() {}
694+
public void onDone(boolean withBeginTransaction) {}
695695

696696
private ResultSet readInternal(
697697
String table,

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ interface Listener {
8484
void onError(SpannerException e, boolean withBeginTransaction);
8585

8686
/** Called when the read finishes normally. */
87-
void onDone();
87+
void onDone(boolean withBeginTransaction);
8888
}
8989

9090
@VisibleForTesting
@@ -118,6 +118,11 @@ public boolean next() throws SpannerException {
118118
ResultSetMetadata metadata = iterator.getMetadata();
119119
if (metadata.hasTransaction()) {
120120
listener.onTransactionMetadata(metadata.getTransaction());
121+
} else if (iterator.isWithBeginTransaction()) {
122+
// The query should have returned a transaction.
123+
throw SpannerExceptionFactory.newSpannerException(
124+
ErrorCode.FAILED_PRECONDITION,
125+
"Query requested a transaction to be started, but no transaction was returned");
121126
}
122127
currRow = new GrpcStruct(iterator.type(), new ArrayList<>());
123128
}
@@ -126,8 +131,10 @@ public boolean next() throws SpannerException {
126131
statistics = iterator.getStats();
127132
}
128133
return hasNext;
129-
} catch (SpannerException e) {
130-
throw yieldError(e, iterator.isWithBeginTransaction() && currRow == null);
134+
} catch (Throwable t) {
135+
throw yieldError(
136+
SpannerExceptionFactory.asSpannerException(t),
137+
iterator.isWithBeginTransaction() && currRow == null);
131138
}
132139
}
133140

@@ -139,6 +146,7 @@ public ResultSetStats getStats() {
139146

140147
@Override
141148
public void close() {
149+
listener.onDone(iterator.isWithBeginTransaction());
142150
iterator.close("ResultSet closed");
143151
closed = true;
144152
}
@@ -150,8 +158,8 @@ public Type getType() {
150158
}
151159

152160
private SpannerException yieldError(SpannerException e, boolean beginTransaction) {
153-
close();
154161
listener.onError(e, beginTransaction);
162+
close();
155163
throw e;
156164
}
157165
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,19 @@ public void onError(SpannerException e, boolean withBeginTransaction) {
539539
}
540540
}
541541

542+
@Override
543+
public void onDone(boolean withBeginTransaction) {
544+
if (withBeginTransaction
545+
&& transactionIdFuture != null
546+
&& !this.transactionIdFuture.isDone()) {
547+
// Context was done (closed) before a transaction id was returned.
548+
this.transactionIdFuture.setException(
549+
SpannerExceptionFactory.newSpannerException(
550+
ErrorCode.FAILED_PRECONDITION,
551+
"ResultSet was closed before a transaction id was returned"));
552+
}
553+
}
554+
542555
@Override
543556
public void buffer(Mutation mutation) {
544557
synchronized (lock) {

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/GrpcResultSetTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public void onTransactionMetadata(Transaction transaction) throws SpannerExcepti
6262
public void onError(SpannerException e, boolean withBeginTransaction) {}
6363

6464
@Override
65-
public void onDone() {}
65+
public void onDone(boolean withBeginTransaction) {}
6666
}
6767

6868
@Before

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/InlineBeginTransactionTest.java‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.google.cloud.spanner;
1818

19-
import static com.google.cloud.spanner.MockSpannerTestUtil.SELECT1;
2019
import static com.google.common.truth.Truth.assertThat;
2120
import static org.junit.Assert.fail;
2221

@@ -37,6 +36,7 @@
3736
import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
3837
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
3938
import com.google.cloud.spanner.TransactionRunnerImpl.TransactionContextImpl;
39+
import com.google.common.base.Predicate;
4040
import com.google.common.base.Throwables;
4141
import com.google.common.collect.ImmutableList;
4242
import com.google.common.util.concurrent.MoreExecutors;
@@ -204,6 +204,7 @@ public void setUp() throws IOException {
204204
public void tearDown() throws Exception {
205205
spanner.close();
206206
mockSpanner.reset();
207+
mockSpanner.clearRequests();
207208
}
208209

209210
@Test
@@ -1348,6 +1349,69 @@ public Void run(TransactionContext transaction) throws Exception {
13481349
assertThat(countRequests(CommitRequest.class)).isEqualTo(0);
13491350
}
13501351

1352+
@Test
1353+
public void testCloseResultSetWhileRequestInFlight() {
1354+
DatabaseClient client = spanner.getDatabaseClient(DatabaseId.of("p", "i", "d"));
1355+
final ExecutorService service = Executors.newSingleThreadExecutor();
1356+
try {
1357+
client
1358+
.readWriteTransaction()
1359+
.run(
1360+
new TransactionCallable<Void>() {
1361+
@Override
1362+
public Void run(TransactionContext transaction) throws Exception {
1363+
final ResultSet rs = transaction.executeQuery(SELECT1);
1364+
// Prevent the server from executing the query.
1365+
mockSpanner.freeze();
1366+
service.submit(
1367+
new Runnable() {
1368+
@Override
1369+
public void run() {
1370+
// This call will be stuck on the server until the mock server is
1371+
// unfrozen.
1372+
rs.next();
1373+
}
1374+
});
1375+
1376+
// Close the result set while the request is in flight.
1377+
mockSpanner.waitForRequestsToContain(
1378+
new Predicate<AbstractMessage>() {
1379+
@Override
1380+
public boolean apply(AbstractMessage input) {
1381+
return input instanceof ExecuteSqlRequest
1382+
&& ((ExecuteSqlRequest) input).getTransaction().hasBegin();
1383+
}
1384+
},
1385+
100L);
1386+
rs.close();
1387+
// The next statement should now fail before it is sent to the server because the
1388+
// first statement failed to return a transaction while the result set was still
1389+
// open.
1390+
mockSpanner.unfreeze();
1391+
try {
1392+
transaction.executeUpdate(UPDATE_STATEMENT);
1393+
fail("missing expected exception");
1394+
} catch (SpannerException e) {
1395+
assertThat(e.getErrorCode()).isEqualTo(ErrorCode.FAILED_PRECONDITION);
1396+
assertThat(e.getMessage())
1397+
.contains("ResultSet was closed before a transaction id was returned");
1398+
}
1399+
return null;
1400+
}
1401+
});
1402+
fail("missing expected exception");
1403+
} catch (SpannerException e) {
1404+
// The commit request will also fail, which means that the entire transaction will fail.
1405+
assertThat(e.getErrorCode()).isEqualTo(ErrorCode.FAILED_PRECONDITION);
1406+
assertThat(e.getMessage())
1407+
.contains("ResultSet was closed before a transaction id was returned");
1408+
}
1409+
service.shutdown();
1410+
assertThat(countRequests(BeginTransactionRequest.class)).isEqualTo(0);
1411+
assertThat(countRequests(ExecuteSqlRequest.class)).isEqualTo(1);
1412+
assertThat(countRequests(CommitRequest.class)).isEqualTo(0);
1413+
}
1414+
13511415
private int countRequests(Class<? extends AbstractMessage> requestType) {
13521416
int count = 0;
13531417
for (AbstractMessage msg : mockSpanner.getRequests()) {

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/ReadFormatTestRunner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public void onTransactionMetadata(Transaction transaction) throws SpannerExcepti
5050
public void onError(SpannerException e, boolean withBeginTransaction) {}
5151

5252
@Override
53-
public void onDone() {}
53+
public void onDone(boolean withBeginTransaction) {}
5454
}
5555

5656
public ReadFormatTestRunner(Class<?> clazz) throws InitializationError {

0 commit comments

Comments
 (0)