File tree

6 files changed

+93
-8
lines changed

6 files changed

+93
-8
lines changed
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,
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
}
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) {
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
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()) {
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)