Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

Changes to Prepare.md to compensate for unintended behaviour #85

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

looopTools
Copy link

By using defer tx.Rollback() a rollback will always be attempted when a function returns, which in my opinion is seems as unintended.
Furthermore, using log.Fatal actually terminates the execution, which rarely is what you want to happen.

prepared.md Outdated Show resolved Hide resolved
@jeffwidman
Copy link

nudge @looopTools

Copy link

@gkristic-vc gkristic-vc left a comment

Choose a reason for hiding this comment

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

Every transaction must end with either a Commit or Rollback. Deferring the Rollback is a safeguard to make sure it's called no matter what. The alternative is that you Rollback in every single error return. Miss one and you'll leak the transaction. Even if you diligently add it to all returns, you still have the problem of a potential panic that your application recovers later on. Deferring allows you to handle all this.

The problem of "always" running a rollback is not really an issue. It may seem counterintuitive at first, but database/sql actually guarantees that once the transaction is first closed (either by Commit or Rollback), subsequent attempts at closing it will be gracefully ignored. From the docs:

After a call to Commit or Rollback, all operations on the transaction fail with ErrTxDone.

This means that the only thing you need to worry about is the Commit when you're ready to do so. Any other path will automatically get a Rollback and, if you actually reached the Commit, the deferred Rollback will be a harmless no-op.

As for the Error versus Fatal... you're right, a typical application won't simply exit. Examples are typically a bit vague on error handling to avoid introducing complexity that's not really the point and may become a distraction. In this case the change to Error is not always correct either. If you failed to db.Begin a transaction, then you can't proceed and try to tx.Prepare. (I mean... you can, but it's not a good practice. It will also fail at best, or maybe even crash your application.) Likewise, if you couldn't prepare the stmt, then trying to Exec it is not going to work. Using log.Fatal is a simplification in terms of error handling, assuming that people can fill in the appropriate handling. We could put a return clause here instead if it helps. We need to show that this flow can't/shouldn't be continued, whether by returning or aborting the application. Changing to log.Error is actually doing a disfavor in my opinion.

Co-authored-by: xaprb <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants