-
Notifications
You must be signed in to change notification settings - Fork 15
fix: rollback transaction on is_dialect exceptions #341
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
Conversation
c29ee4e to
6b5f1e6
Compare
6b5f1e6 to
4f5062a
Compare
| except Exception: | ||
| pass | ||
| except Exception as e: | ||
| if self.exception_handler and self.exception_handler.is_network_exception(e): |
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.
We could get here under two scenarios:
- This is not the right database dialect so the SQL we executed failed
- This is the right database dialect but some other failure occurred
In scenario 1, it seems a bit weird to use this database dialect's exception handler since this actually isn't the correct database dialect. Not sure what the best solution would be though or if this code would actually cause any problems since our exception handlers aren't much different from each other
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 it will be okay in scenario 1. If the is_network_exception is failed, we return a false, the driver will continue to iterate through the loop calling is_dialect, which will all fail due to network errors, until eventually we get to the right dialect.
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.
My main concern would be this scenario:
- the network is up but this isn't the right database dialect so the SQL fails
- we hit this line and check for a network exception. The actual database dialect's exception handler would properly determine that this is not a network exception. But the current, incorrect database dialect's exception handler incorrectly concludes that this is a network exception
- the SQL exception is raised to the user
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.
Right now I don't think this would happen in practice but if we change exception handler code or add new exception handlers it could theoretically happen
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.
Chatted offline with @congoamz, the decision is to check for the network exception and attempt a rollback regardless. If the exception was in fact a network exception the rollback would either:
a) fail and the error will be propagated, or
b) the rollback call timeouts and a query timeout error will be thrown
Description
Autocommit is set to false by default in Python. Our internal queries may start a new transaction.
The driver checks for the correct database dialect by executing internal queries, if the query fails due to syntax error then we'd know that this is the incorrect database dialect. However, when the driver starts a transaction during internal query executions and an error was thrown, if we don't rollback the transaction the connection would be unusable later on.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.