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: kill connection on commit/rollback error #14535

Merged
merged 7 commits into from May 27, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented May 24, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This is an attempt at fixing #10858. I don't know if it'll completely fix it, so in doubt I'm not closing that issue.

What was going on?

If an error occurred during a transaction commit or rollback (such as a network error), Sequelize would call transaction.cleanup(), which released the connection to the pool.

The problem is that if an error occurred, we cannot guarantee that the transaction did actually commit or roll back. It may still be active. Releasing the connection to the pool does not kill the connection, and it will end up used by other queries - unknowingly running in another transaction.

The fix

This PR changes that by taking the drastic approach of killing the connection if an error occurred during COMMIT TRANSACTION or ROLLBACK TRANSACTION.

@ephys ephys self-assigned this May 24, 2022
@ephys ephys requested a review from a team May 24, 2022 18:10
@sdepold
Copy link
Member

sdepold commented May 24, 2022

Regarding the test, I guess you could stub rollbackTransaction / commitTransaction during a test?

@ephys
Copy link
Member Author

ephys commented May 24, 2022

Yes, I think that's how I'll do that :)

@ephys ephys requested a review from sdepold May 25, 2022 08:28
@sdepold sdepold self-requested a review May 25, 2022 16:48
sdepold
sdepold previously approved these changes May 25, 2022
@sdepold
Copy link
Member

sdepold commented May 25, 2022

I approved it. Don't know how strong I feel about the .length change. You decide

@ephys ephys merged commit e1a9c28 into v6 May 27, 2022
@ephys ephys deleted the ephys/kill-connection-on-commit-error branch May 27, 2022 15:52
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.20.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants