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.
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
fix: handle thrown exceptions in runAsyncTransaction callback #671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look at
java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java
Line 465 in f2c76a1
private static class TransactionAsyncAdapter<T> implements Transaction.AsyncFunction<T> { |
I saw that, but it's only clean for synchronous functions. For async callbacks that return an ApiFuture, I think the only way is to add a callback. |
} | ||
try { | ||
ApiFutures.addCallback( | ||
userCallback.updateCallback(transaction), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #667.
The callback
onFailure
is only triggered if the ApiFuture contains the exception. When a runtime exception is thrown neither theonFailure
noronSuccess
handler is called, leading the ApiFuture to hang and never complete.The fix adds a
catchingAsync()
aroundupdateCallback
to guard against thrown exceptions. Added a system test for redundancy, but I can remove it. Both tests fail (never complete) when run against master.