Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: handle thrown exceptions in runAsyncTransaction callback #671

Merged
merged 5 commits into from Jun 25, 2021
Merged

fix: handle thrown exceptions in runAsyncTransaction callback #671

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b07579
fix: handle thrown exceptions in runAsyncTransaction callback
Jun 18, 2021
0be3825
add try/catch blocks
Jun 24, 2021
c7de663
removed unnecessary catchingAsync()
Jun 24, 2021
a1f7709
separate out callback
Jun 25, 2021
e122a93
jk, separate it another way
Jun 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 18 additions & 13 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java
View file
Open in desktop
Expand Up @@ -146,24 +146,29 @@ public void run() {
*/
private SettableApiFuture<T> invokeUserCallback() {
final SettableApiFuture<T> callbackResult = SettableApiFuture.create();

userCallbackExecutor.execute(
new Runnable() {
@Override
public void run() {
ApiFutures.addCallback(
userCallback.updateCallback(transaction),
new ApiFutureCallback<T>() {
@Override
public void onFailure(Throwable t) {
callbackResult.setException(t);
}
try {
ApiFutures.addCallback(
userCallback.updateCallback(transaction),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code would be easier to understand if you did something like this:

try {
const result = userCallback.updateCallback(transaction)
ApiFutures.addCallback(...)
} catch (Exception e) {
   callbackResult.setException(e);
}

It's very hard to tell what the try/catch block addresses. The other alternative is to wrap the callback in a try/catch before passing it here like in TransactionAsyncAdapter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, moved it out into a separate variable.

new ApiFutureCallback<T>() {
@Override
public void onFailure(Throwable t) {
callbackResult.setException(t);
}

@Override
public void onSuccess(T result) {
callbackResult.set(result);
}
},
MoreExecutors.directExecutor());
@Override
public void onSuccess(T result) {
callbackResult.set(result);
}
},
firestoreExecutor);
} catch (Exception e) {
callbackResult.setException(e);
}
}
});
return callbackResult;
Expand Down
27 changes: 26 additions & 1 deletion google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java
View file
Open in desktop
Expand Up @@ -253,7 +253,7 @@ public String updateCallback(Transaction transaction) throws Exception {
}

@Test
public void rollbackOnCallbackErrorAsync() {
public void rollbackOnCallbackApiFutureErrorAsync() {
doReturn(beginResponse())
.doReturn(rollbackResponse())
.when(firestoreMock)
Expand Down Expand Up @@ -339,6 +339,31 @@ public ApiFuture<String> updateCallback(Transaction transaction) {
assertEquals(1, requests.size());
}

@Test
public void noRollbackOnThrownExceptionAsync() {
doReturn(beginResponse())
.doReturn(rollbackResponse())
.when(firestoreMock)
.sendRequest(requestCapture.capture(), Matchers.<UnaryCallable<Message, Message>>any());

ApiFuture<String> transaction =
firestoreMock.runAsyncTransaction(
new Transaction.AsyncFunction<String>() {
@Override
public ApiFuture<String> updateCallback(Transaction transaction) {
throw new RuntimeException("User exception");
}
},
options);

try {
transaction.get();
fail();
} catch (Exception e) {
assertTrue(e.getMessage().endsWith("User exception"));
}
}

@Test
public void limitsRetriesWithFailure() {
ResponseStubber responseStubber =
Expand Down
18 changes: 18 additions & 0 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java
View file
Open in desktop
Expand Up @@ -688,6 +688,24 @@ public String updateCallback(Transaction transaction) {
}
}

@Test
public void asyncTxFailsWithUserError() throws Exception {
try {
firestore
.runAsyncTransaction(
new Transaction.AsyncFunction<String>() {
@Override
public ApiFuture<String> updateCallback(Transaction transaction) {
throw new RuntimeException("User exception");
}
})
.get();
fail();
} catch (Exception e) {
assertTrue(e.getMessage().endsWith("User exception"));
}
}

@Test
public void doesNotRetryTransactionsWithFailedPreconditions() {
final DocumentReference documentReference = randomColl.document();
Expand Down