fix(mysql, mariadb): release connection on deadlocks by hsource · Pull Request #12841 · sequelize/sequelize

hsource · 2020-11-16T10:23:06Z

Pull Request check-list

Please make sure to review and check all of these items:

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

Description of change

This change makes deadlocked queries release the MariaDB/MySQL connection and does a redundant rollback. These ensure that subsequent queries work properly.

Debugging

While running sequelize in production for a collaborative document editor, we found that deadlocks on MariaDB would sometimes cause other queries to stall. After debugging, we realized that this is because MySQL/MariaDB has custom deadlock handling in Sequelize.

Because InnoDB (MariaDB/MySQL engine) automatically rolls back one query in the case of deadlocks, sequelize marks the transaction.finished = 'rollback' in the run function of query.js
In transaction() of sequelize.js, transaction.rollback() isn't called if transaction.finished is set
As such, transaction.cleanup(), which releases the connection is never called for this connection, and can cause hangs in this case

Fix

The fix was to ensure we call transaction.cleanup(), even if we don't rollback the transaction. I added a test for this case too.

Interestingly, on MariaDB, we still need to do an explicit rollback query. Without it, a follow-up READ COMMITTED transaction wouldn't behave as expected. I added this to the testcase too.

codecov · 2020-11-16T23:21:30Z

Codecov Report

Merging #12841 (828430e) into master (2de3377) will decrease coverage by 0.12%.
The diff coverage is 95.80%.

@@            Coverage Diff             @@
##           master   #12841      +/-   ##
==========================================
- Coverage   96.44%   96.32%   -0.13%     
==========================================
  Files          95       95              
  Lines        9154     9262     +108     
  Branches        0       90      +90     
==========================================
+ Hits         8829     8922      +93     
+ Misses        325      323       -2     
- Partials        0       17      +17

Impacted Files	Coverage Δ
index.js	`100.00% <ø> (ø)`
src/associations/index.js	`100.00% <ø> (ø)`
src/dialects/abstract/connection-manager.js	`91.80% <0.00%> (ø)`
src/dialects/abstract/index.js	`100.00% <ø> (ø)`
src/dialects/abstract/query-generator/operators.js	`90.90% <ø> (ø)`
...c/dialects/abstract/query-generator/transaction.js	`87.50% <ø> (ø)`
src/dialects/mssql/index.js	`100.00% <ø> (ø)`
src/dialects/mysql/data-types.js	`98.43% <ø> (ø)`
src/dialects/mysql/index.js	`100.00% <ø> (ø)`
src/dialects/parserStore.js	`100.00% <ø> (ø)`
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2de3377...a494b7e. Read the comment docs.

papb

Hi! Thanks for the PR!

test/integration/transaction.test.js

src/transaction.js

hsource · 2020-11-18T11:10:02Z

Thanks for the prompt review! I improved the comments and pushed some new changes. @papb - feel free to take another look

hsource · 2020-12-11T19:40:47Z

@papb - quick ping on this - can you take a look? We're currently using our own branch, and would love to get this checked in so we can get back to the normal releases.

hsource · 2020-12-19T00:22:25Z

Quick follow-up ping! @papb - thanks again!

ckeboss · 2021-01-02T19:34:26Z

Looks like this is more robust than #12588

papb · 2021-01-25T12:50:09Z

Hello!! Thank you very much for this super high quality PR. Great work! Sorry to take so long!

papb

Great work!!

github-actions · 2021-01-27T04:07:36Z

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

This is a follow-up for a problem not covered by #12841 This commit also contains some refactoring.

arbieo · 2021-02-22T00:48:14Z

Thanks for this. I believe the scope of this fix includes postgres as well. We were seeing unreleased connections after deadlocks caused by non-transaction calls within transactions.

papb · 2021-02-24T13:43:30Z

Hi @arbieo, if I understand correctly you are saying this PR also fixed a similar postgres issue? Great news then :)

arbieo · 2021-02-24T14:27:39Z

@papb Correct. I made a tentative fix in my local for the issue we had and then found this online afterwards that is largely the same (I probably should have checked here first).

This is a follow-up for a problem not covered by #12841.

* test(mysql, mariadb): improve transaction tests - Greatly improve test for `SELECT ... LOCK IN SHARE MODE` - Greatly improve test for deadlock handling * fix(mysql): release connection on deadlocks This is a follow-up for a problem not covered by #12841. * refactor(mariadb): `query.js` similar to mysql's * Update comments with a reference to this PR

* test(mysql, mariadb): improve transaction tests - Greatly improve test for `SELECT ... LOCK IN SHARE MODE` - Greatly improve test for deadlock handling * fix(mysql): release connection on deadlocks This is a follow-up for a problem not covered by sequelize#12841. * refactor(mariadb): `query.js` similar to mysql's * Update comments with a reference to this PR

hsource force-pushed the harry-deadlocks branch from 4f99328 to c900b71 Compare November 16, 2020 20:16

papb requested changes Nov 17, 2020

View reviewed changes

test/integration/transaction.test.js Show resolved Hide resolved

test/integration/transaction.test.js Show resolved Hide resolved

test/integration/transaction.test.js Outdated Show resolved Hide resolved

src/transaction.js Show resolved Hide resolved

hsource force-pushed the harry-deadlocks branch from 079a7bc to 0c1b449 Compare November 18, 2020 19:50

hsource requested a review from papb November 19, 2020 23:43

hsource mentioned this pull request Jan 10, 2021

mariadb transaction deadlock does not release connection #11571

Closed

7 tasks

hsource added 2 commits January 25, 2021 10:05

fix(mysql, mariadb): release connection on deadlocks

88de48c

fix(mysql, mariadb): added better comments about transaction changes

a494b7e

papb force-pushed the harry-deadlocks branch from 828430e to a494b7e Compare January 25, 2021 13:25

papb changed the base branch from master to main January 25, 2021 13:25

papb approved these changes Jan 25, 2021

View reviewed changes

papb merged commit c77b1f3 into sequelize:main Jan 25, 2021

papb mentioned this pull request Jan 25, 2021

Cleanup connection if deadlock occurs in mariadb or mysql. Fixes #11571 #12588

Closed

6 tasks

github-actions bot added the released label Jan 27, 2021

papb added a commit that referenced this pull request Feb 1, 2021

fix(mysql): release connection on deadlocks

2a6d617

This is a follow-up for a problem not covered by #12841 This commit also contains some refactoring.

papb added a commit that referenced this pull request Mar 14, 2021

fix(mysql): release connection on deadlocks

cb3574c

This is a follow-up for a problem not covered by #12841.

papb mentioned this pull request Mar 14, 2021

fix(mysql): release connection on deadlocks #13102

Merged

jctaveras pushed a commit to jctaveras/sequelize that referenced this pull request Mar 29, 2021

fix(mysql, mariadb): release connection on deadlocks (sequelize#12841)

392fdd6

matthewpalmer mentioned this pull request Jul 25, 2023

Change in transaction behavior in Sequelize 6.5 and up #16312

Closed

2 tasks

Uh oh!

Conversation

hsource commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request check-list

Description of change

Debugging

Fix

Uh oh!

codecov bot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

papb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hsource commented Nov 18, 2020

Uh oh!

hsource commented Dec 11, 2020

Uh oh!

hsource commented Dec 19, 2020

Uh oh!

ckeboss commented Jan 2, 2021

Uh oh!

papb commented Jan 25, 2021

Uh oh!

papb left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 27, 2021

Uh oh!

arbieo commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

papb commented Feb 24, 2021

Uh oh!

arbieo commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hsource commented Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

arbieo commented Feb 22, 2021 •

edited

Loading