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.

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

Merged
merged 5 commits into from Jun 25, 2021

Conversation

thebrianchen
Copy link

Fixes #667.

The callback onFailure is only triggered if the ApiFuture contains the exception. When a runtime exception is thrown neither the onFailure nor onSuccess handler is called, leading the ApiFuture to hang and never complete.

The fix adds a catchingAsync() around updateCallback 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.

@thebrianchen thebrianchen requested a review from a team as a code owner June 18, 2021 22:06
@thebrianchen thebrianchen requested a review from a team June 18, 2021 22:06
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Jun 18, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 18, 2021
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a 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

private static class TransactionAsyncAdapter<T> implements Transaction.AsyncFunction<T> {
? It might be cleaner to follow the same pattern here.

@thebrianchen
Copy link
Author

? It might be cleaner to follow the same pattern here.

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),
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.

@thebrianchen thebrianchen merged commit 969f7fd into master Jun 25, 2021
@thebrianchen thebrianchen deleted the bc/async-tx branch June 25, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

firestore-admin: runAsyncTransaction() throwing exception never stops retrying
2 participants