fix(mysql): deadlocks indicate a rollback #11074
Conversation
|
I don't think the test failures are related to this change. They pass locally for me. |
| @@ -614,24 +614,30 @@ class Sequelize { | |||
| [sql, bindParameters] = this.dialect.Query.formatBindParameters(sql, options.bind, this.options.dialect); | |||
| } | |||
|
|
|||
| const checkTransaction = () => { | |||
| if (options.transaction && options.transaction.finished && !options.completesTransaction) { | |||
There was a problem hiding this comment.
Why completesTransaction is added to this check? Is it to let rollback and commit run on already finished transaction?
There was a problem hiding this comment.
It's because we don't want to actually run this check against a COMMIT or ROLLBACK query, because transaction.finished actually gets set before this is invoked.
lib/sequelize.js
Outdated
|
|
||
| return options.transaction | ||
| ? options.transaction.connection | ||
| : this.connectionManager.getConnection(options); | ||
| }).then(connection => { | ||
| const query = new this.dialect.Query(connection, this, options); | ||
| return this.runHooks('beforeQuery', options, query) | ||
| .tap(() => checkTransaction()) |
There was a problem hiding this comment.
Why checkTransaction is called here? To check deadlocks caused by beforeQuery?
Also just use then here, we aren't using return value in promise chain
There was a problem hiding this comment.
Correct. I wanted to move this check as close to the actual execution of the query as possible.
If a commit/rollback/deadlock happens during getConnection() or beforeQuery, I don't want to mistakenly run a query outside of a transaction.
Because beforeQuery and getConnection() can both be asynchronous, I'm actually more concerned about something in the background finishing the transaction, rather than beforeQuery committing the transaction itself.
I'll change this to a then.
| @@ -70,7 +70,52 @@ if (dialect === 'mysql') { | |||
| reltype: 'child' | |||
| })); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Deadlock in transaction', () => { | |||
There was a problem hiding this comment.
Use this testcase, modify as per your requirements. Add this test-case to https://github.com/sequelize/sequelize/blob/master/test/integration/transaction.test.js#L368
if (dialect === 'mysql' || dialect === 'mariadb') {
describe('deadlock handling', () => {
it('should treat deadlocked transaction as rollback', function () {
const Task = this.sequelize.define('task', {
id: {
type: Sequelize.INTEGER,
primaryKey: true
}
});
// This gets called twice simultaneously, and we expect at least one of the calls to encounter a
// deadlock (which effectively rolls back the active transaction).
// We only expect createTask() to insert rows if a transaction is active. If deadlocks are handled
// properly, it should only execute a query if we're actually inside a real transaction. If it does
// execute a query, we expect the newly-created rows to be destroyed when we forcibly rollback by
// throwing an error.
// tl;dr; This test is designed to ensure that this function never inserts and commits a new row.
const update = (from, to) => this.sequelize.transaction(transaction => {
return Task.findAll({
where: {
id: {
[Sequelize.Op.eq]: from
}
},
lock: transaction.LOCK.UPDATE,
transaction
})
.then(() => Promise.delay(10))
.then(() => {
return Task.update({ id: to }, {
where: {
id: {
[Sequelize.Op.ne]: to
}
},
lock: transaction.LOCK.UPDATE,
transaction
});
})
.catch(e => { console.log(e.message); })
.then(() => Task.create({ id: 2 }, { transaction }))
.catch(e => { console.log(e.message); })
.then(() => { throw new Error('Rollback!'); });
}).catch(() => {});
return this.sequelize.sync({ force: true })
.then(() => Task.create({ id: 0 }))
.then(() => Task.create({ id: 1 }))
.then(() => Promise.all([
update(0, 1),
update(1, 0)
]))
.then(() => {
return Task.count().then(count => {
// If we were actually inside a transaction when we called `Task.create({ id: 2 })`, no new rows should be added.
expect(count).to.equal(2, 'transactions were fully rolled-back, and no new rows were added');
});
});
});
});
}There was a problem hiding this comment.
Okay, I'll move the test into the other file. Thanks for the feedback!
Codecov Report
@@ Coverage Diff @@
## master #11074 +/- ##
==========================================
+ Coverage 96.33% 96.34% +<.01%
==========================================
Files 94 94
Lines 9015 9022 +7
==========================================
+ Hits 8685 8692 +7
Misses 330 330
Continue to review full report at Codecov.
|
schmod
force-pushed
the
mysql-handle-deadlock
branch
from
0db052e
to
3752dc6
Compare
|
🎉 This PR is included in version 5.9.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 7.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
When MySQL or MariaDB encounter a deadlock, the default behavior is to roll back the active transaction (having roughly the same effect as manually issuing the
ROLLBACK;command).Currently, Sequelize does not apply any special handling to these errors. If a query yields a deadlock error, all future queries against that "transaction" will actually be run against the default context. This can have potentially destructive implications, as those subsequent queries cannot be rolled back.
This PR adds some code to mark any deadlocked transactions as though they've been rolled-back. This prevents Sequelize from running any further queries against these transactions (utilizing the mechanisms that already exist for that purpose).
Mitigates #8352.