mariadb transaction deadlock does not release connection

[seally1186]

Issue Description

What are you doing?

Link to SSCCE PR: sequelize/sequelize-sscce#15

"use strict";

const CONFIG = require("../config");
const Sequelize = require("sequelize");

async function main() {
  const sequelize = new Sequelize(CONFIG.dbUrl, {
    pool: { max: 2, acquire: 1000 },
    keepDefaultTimezone: true, timezone: "" /* Suppress timezone warnings */
  });

  await sequelize.query("DROP TABLE IF EXISTS test1");
  await sequelize.query("CREATE TABLE test1 (id INT NOT NULL PRIMARY KEY)");
  await sequelize.query("DROP TABLE IF EXISTS test2");
  await sequelize.query("CREATE TABLE test2 (id INT NOT NULL PRIMARY KEY)");
  await sequelize.query("INSERT INTO test1 VALUES (1)");
  await sequelize.query("INSERT INTO test2 VALUES (1)");

  let t1 = await sequelize.transaction();
  let t2 = await sequelize.transaction();
  await sequelize.query("UPDATE test1 SET id = id WHERE id = 1", { transaction: t1 });
  await sequelize.query("UPDATE test2 SET id = id WHERE id = 1", { transaction: t2 });

  /* This query will wait on t2, don't block on it */
  sequelize.query("UPDATE test2 SET id = id WHERE id = 1", { transaction: t1 });
  try {
    await sequelize.query("UPDATE test1 SET id = id WHERE id = 1", { transaction: t2 });
  } catch (e) {
    console.log(e.message);
    console.log("t2 status is " + t2.finished);
    /* Cannot `t2.rollback()` here because it is marked as finished */
  }
  try {
    let t3 = await sequelize.transaction();
  } catch (e) {
    console.log(e.stack);
  }
}
main().then(() => process.exit(0));

What do you expect to happen?

The second query in t1 will wait for t2, and the second query of t2 will result in a deadlock. After this, t2 should be rolled back, so we should have one available connection to start t3.

What is actually happening?

[...initial setup...]
Executing (68180761-6e97-443c-a070-9ca00810e563): START TRANSACTION;
Executing (50539931-2dd5-4b45-be57-6747c2af47e9): START TRANSACTION;
Executing (68180761-6e97-443c-a070-9ca00810e563): UPDATE test1 SET id = id WHERE id = 1
Executing (50539931-2dd5-4b45-be57-6747c2af47e9): UPDATE test2 SET id = id WHERE id = 1
Executing (68180761-6e97-443c-a070-9ca00810e563): UPDATE test2 SET id = id WHERE id = 1
Executing (50539931-2dd5-4b45-be57-6747c2af47e9): UPDATE test1 SET id = id WHERE id = 1
(conn=5493, no: 1213, SQLState: 40001) Deadlock found when trying to get lock; try restarting transaction
sql: UPDATE test1 SET id = id WHERE id = 1 - parameters:[]
t2 status is rollback
SequelizeConnectionAcquireTimeoutError: Operation timeout
    at [project]/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:282:52

After one second of waiting, the timeout error is thrown for t3.

Additional context

The problem seems to be these lines in mariadb/query.js:

 59            // MariaDB automatically rolls-back transactions in the event of a deadlock
 60            if (options.transaction && err.errno === 1213) {
 61              options.transaction.finished = 'rollback';
 62            }

These mark the transaction as rolled back, but do not call cleanup to release the connection. (Adding t2.cleanup() in the catch seems to solve the problem.)

Environment

• Sequelize version: sequelize@5.19.8
• Node.js version: v12.11.1
• Operating System: Ubuntu

Issue Template Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s): mariadb and mysql
• I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Tested with mariadb, but the same lines appear in mysql/query.js.

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[papb]

Thanks for the detailed report. Can you wrap your code into a sequelize-sscce please?

[seally1186]

Sure, here's the PR: sequelize/sequelize-sscce#15 . I've also cleaned it up to use models rather than raw queries.

[papb]

Fantastic, thanks!

[tmont]

I ran into this same issue. I can confirm the same lack of cleanup occurs on MySQL 5.7 as well.

I can also confirm that it only seems to happen for deadlocks; other types of errors (lock wait timeouts, unique constraint errors) all cleaned up properly.

[seally1186]

Any word on this? I've been running with if (e instanceof db.Sequelize.DatabaseError && e.parent.code === "ER_LOCK_DEADLOCK") await t.cleanup(); in my transaction catch block for a couple months now and it seems to be working.

(I'm not sure if I have the right environment set up to properly test a PR.)

[tmont]

Any word on this? I've been running with if (e instanceof db.Sequelize.DatabaseError && e.parent.code === "ER_LOCK_DEADLOCK") await t.cleanup(); in my transaction catch block for a couple months now and it seems to be working.

I do the exact same thing and haven't seen any problems either.

try {
  // ...
} catch (e) {
    if (e.parent && e.parent.code === 'ER_LOCK_DEADLOCK') {
        (transaction as any).cleanup();
    }
    throw e;
}

I tried to make a PR to fix it at one point, but was a little unclear as to where to put the cleanup, as the transaction-handling stuff seemed to be a little scattered around. I couldn't tell if I was going to accidentally double-clean-up a transaction and cause other errors.

[testn]

@sushantdhiman can you recommend the best way to address this? I can submit the PR

[testn]

@papb Do you mind take a look at #12588?

[ckeboss]

Any update on this? I have made a PR to address this issue.

[prithvin]

As a hack for the time being, are there any downsides to running cleanup on every rolled back / failed transaction?

[hsource]

I didn't realize there was an issue for this already! I fixed this in #12841 and our fix has been running in production on a fork of sequelize for the past few months. All it needs is a review.

@prithvin is correct - we should run cleanup on every succeeded or failed transaction, and that fix ensures it happens.

[github-actions]

🎉 This issue has been resolved in version 6.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀