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: always rollback #638

Merged
merged 2 commits into from
Feb 23, 2022
Merged

fix: always rollback #638

merged 2 commits into from
Feb 23, 2022

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Nov 24, 2021

Related Issue or Design Document

Fixes #637.

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added necessary documentation within the code base (if
    appropriate).

Further comments

There is not much change necessary to get this fixed. See the diff.

There is one logical change I have done: before rollback was not called if commit failed, now I made it that it is called if commit fails. It is generally safe to call rollback outside of a transaction in PostgreSQL (you only get warning), but if the connection is in error state, then rollback is required. So just to be safe, I think it is better to rollback on any error, including when commit fails (because we might not know what is the reason for failure of that call and in what state is the connection).

@aeneasr
Copy link
Member

aeneasr commented Dec 22, 2021

Are you still up for the changes? 🧐 If you need any help, let us know!

I'll convert this to a draft in the meanwhile. When it's ready for review again, please remove the draft indicator! 👀

@aeneasr aeneasr marked this pull request as draft December 22, 2021 15:00
@mitar
Copy link
Contributor Author

mitar commented Dec 22, 2021

Yes yes. I was just a bit busy with other things.

@aeneasr
Copy link
Member

aeneasr commented Dec 29, 2021

All good. I think I am known by now as one of the slowest maintainers in open source 😓

@mitar mitar marked this pull request as ready for review January 14, 2022 22:11
@mitar mitar requested a review from aeneasr January 14, 2022 22:12
@mitar
Copy link
Contributor Author

mitar commented Jan 14, 2022

All good. I think I am known by now as one of the slowest maintainers in open source

What? No way. You are one of best maintainers I have seen around. Regularly checking in, always giving great feedback, pushing PRs through and not leaving them stick around.

@aeneasr
Copy link
Member

aeneasr commented Jan 18, 2022

What? No way. You are one of best maintainers I have seen around. Regularly checking in, always giving great feedback, pushing PRs through and not leaving them stick around.

You made my day, thank you :)

@aeneasr
Copy link
Member

aeneasr commented Jan 18, 2022

Re-running conformity tests that hung up

@mitar
Copy link
Contributor Author

mitar commented Jan 18, 2022

BTW, those tests are failing also in master branch.

@mitar
Copy link
Contributor Author

mitar commented Jan 31, 2022

ping @aeneasr

@aeneasr
Copy link
Member

aeneasr commented Feb 2, 2022

Ah yes it fails because the refresh PR has not yet been merged. I'd like for the OIDC conformity siute to pass first so that we can confirm that the rollback has no unintended effects :)

@mitar
Copy link
Contributor Author

mitar commented Feb 2, 2022

Which other PR?

@aeneasr
Copy link
Member

aeneasr commented Feb 16, 2022

i merged a workaround, let's see if we're green!

@aeneasr aeneasr merged commit 7edf673 into ory:master Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollback is not called if there is a panic
2 participants