Rollback is not called if there is a panic

[mitar]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• This issue affects my Ory Cloud project.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Describe the bug

Current use of BeginTx, CommitTx and RollbackTx in Fosite is problematic because it does not call RollbackTx when panic happens. We could assume that Fosite itself does not panic, but that is not necessary true for code it calls into. For example, I had a bug inside my storage implementation which triggered panic. As the panic propagated and was handled higher up in my request handler and logged, transaction itself was not rolled back, leaving the connection over it in an inconsistent state. With connection pooling that brought some stability issues.

Reproducing the bug

Sadly I do not have a simple reproduction here. One which I was able to do is with Sqlite and setting SetMaxOpenConns to 1. Then if a transaction is not rolled back, no other operation can be done on the database and my code hangs. This is how I am now testing that all transactions are properly rolled back.

Relevant log output

No response

Relevant configuration

No response

Version

observed on current master branch

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

[aeneasr]

This is also the case for os.Exit, *.Fatal* and other exit scenarios such as unhandled SIGHUP. Most of these we can't detect. IMO catching panics isn't solvable without a substantial transaction refactor. Keep in mind that fosite has transactions optionally available, some storage engines don't even support transactions! The question is if that's worth the effort, as panics are rare and retries are cheap ("oh the server crashed, guess I need to try and refresh the token again").

[mitar]

I do not think perfect (catching all possible ways code can exist) should be the enemy of good: catching a standard way for Go code to throw an exception when there is a bug in the code it is calling into, and cleaning up. In fact, Go gives you a nice way to cleanup. I would even say that it would simplify existing code.

See here. There is so many storage.MaybeRollbackTx cases in there. All that could be moved into one defer and this is it.

I can try to do a MR if it would be an acceptable change.