Concurrent nested transactions are not compatible with connection pool of finite size

[misos1]

What are you doing?

When using connection pool with maximum number of connections then there should not be "dependencies" between connections or else deadlock can happen. I suppose concurrent nested transactions is feature of sequelize as they are mentioned in documentation: http://docs.sequelizejs.com/manual/transactions.html#concurrent-partial-transactions

I borrowed example from documentation, deleted some lines and created function nested from it:

let Sequelize = require("sequelize");
let sequelize = new Sequelize({ dialect: "mysql" });

let nested = function()
{
	return sequelize.transaction((t1) => {
		return sequelize.transaction((t2) => {
		});
	});
};
Promise.all([nested(), nested(), nested(), nested(), nested()]).then(() => console.log("done"));

Output can be similar to this:

22:29:24.751 - Executing (52c933c4-50dc-49e8-8d34-11e1e916e920): START TRANSACTION;
22:29:24.758 - Executing (0009969f-e4a0-4a1e-ad55-967bb3beea44): START TRANSACTION;
22:29:24.760 - Executing (5729d881-fa7c-4bb0-b02f-d1618740bc34): START TRANSACTION;
22:29:24.762 - Executing (9060a023-455b-49f7-bcbb-2ccefe9b6685): START TRANSACTION;
22:29:24.763 - Executing (0fd8646b-e298-4989-922e-16d546897d6f): START TRANSACTION;
22:30:24.762 - Executing (52c933c4-50dc-49e8-8d34-11e1e916e920): ROLLBACK;
22:30:24.765 - Executing (17dad8f9-0cde-406d-8bb6-51a21b1c17dd): START TRANSACTION;
Unhandled rejection SequelizeConnectionAcquireTimeoutError: Operation timeout
    at pool.acquire.catch.err 
    at tryCatcher
    ...
22:30:24.771 - Executing (17dad8f9-0cde-406d-8bb6-51a21b1c17dd): COMMIT;
22:30:24.775 - Executing (5729d881-fa7c-4bb0-b02f-d1618740bc34): ROLLBACK;
22:30:24.777 - Executing (9060a023-455b-49f7-bcbb-2ccefe9b6685): ROLLBACK;
22:30:24.778 - Executing (0fd8646b-e298-4989-922e-16d546897d6f): ROLLBACK;
22:30:24.782 - Executing (0009969f-e4a0-4a1e-ad55-967bb3beea44): COMMIT;

Naive solution would be to increase maximum number of connections but none finite value will work well in theory when number of concurrent requests/calls to function nested is also not limited so the best solution would be to not limit number of connections in pool or to not use concurrent nested transactions. But then is lost benefit of having limited number of db connections. Sequelize should see these types of dependencies and maybe is possible to avoid such deadlocks. Maybe multiple pools could help one for each nesting and option in seuqelize config for (maximum) number of pools or create new pools automatically to avoid deadlocks (or resize pool temporarily above max number of connections in case of deadlock).

To Reproduce
Steps to reproduce the behavior:

1. Setup sequelize with maximum number of connections in pool set to 5 (default as of writing this)
2. Run example code
3. See error

What do you expect to happen?

To not see deadlock and this error.

What is actually happening?

Deadlock and sequelize error.

Environment

Dialect:

• mysql
• postgres
• sqlite
• mssql
• any
  Sequelize version: 5.8.6

[papb]

Can you create a MCVE that does not rely on CLS, please?

[misos1]

You mean to remove that comment about CLS? Ok done.

Result is exactly same whether I setup sequelize with or without CLS.

I just copied part of example from sequelize documentation with headings "Concurrent/Partial transactions" and "Without CLS enabled".

[papb]

Oh, sorry about that. My mistake. Thanks!!

[ivolucien]

FWIW, native "nested" transactions for mysql, mariadb and other dbs are approximated within the same connection by using SAVEPOINT foo ... ROLLBACK TO foo, support for those would be wonderful.

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[misos1]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

Should we now ping every one of our issues every 14 days? (This comment did not remove the "stale" label.)

[WikiRik]

The bot will remove the stale label when it runs again tonight. There is work being done that you don't have to ping every one of your issues every 14 days. That is part of #13648

[ephys]

I believe the updated documentation & added transaction features covers the issues raised in this issue: https://sequelize.org/docs/v7/querying/transactions/#nested-transactions

If not, please let us know

[misos1]

@ephys It is same as before, nothing changed, there is still deadlock probably due to exactly same reasons as before, sequelize version 6.37.1:

Executing (a50e6b99-70d0-4a98-9d56-b0542c2ec2dc): START TRANSACTION;
Executing (5b86a9e0-47e0-44fe-a932-ddad525ce1d3): START TRANSACTION;
Executing (52ede8dd-44a2-44c0-b52e-59767bd688ed): START TRANSACTION;
Executing (0bb46276-9047-4026-9d0f-d99a114e23c4): START TRANSACTION;
Executing (2cf11080-6eb4-4196-a798-b666d7eff609): START TRANSACTION;
Executing (a50e6b99-70d0-4a98-9d56-b0542c2ec2dc): ROLLBACK;
Executing (5b86a9e0-47e0-44fe-a932-ddad525ce1d3): ROLLBACK;
Executing (07811b6a-f558-45d4-b265-fc96fc0c6664): START TRANSACTION;
~/.tmp/aaa/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:206
        throw new errors.ConnectionAcquireTimeoutError(error);
              ^

ConnectionAcquireTimeoutError [SequelizeConnectionAcquireTimeoutError]: Operation timeout
    at ConnectionManager.getConnection (~/.tmp/aaa/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:206:15)

[ephys]

The documentation in question is Sequelize 7, this can't be changed in Sequelize 6

In the new version, nesting transactions re-uses the same transaction by default. Creating a new transaction requires opting into that nest mode, as shown here.

In this instance, if you actively opt into creating separate but dependent transactions until you run out of connections, you will have a deadlock. It's not something we can stop you from doing, but it's using transactions incorrectly. We can only provide safer defaults and document the risks of the alternative solutions, which is what we did for v7

[misos1]

The documentation in question is Sequelize 7, this can't be changed in Sequelize 6

In the new version, nesting transactions re-uses the same transaction by default. Creating a new transaction requires opting into that nest mode, as shown here.

In this instance, if you actively opt into creating separate but dependent transactions until you run out of connections, you will have a deadlock. It's not something we can stop you from doing, but it's using transactions incorrectly. We can only provide safer defaults and document the risks of the alternative solutions, which is what we did for v7

@ephys How is that usage incorrect? It results in deadlock only because of internal implementation detail of sequelize that is not documented. I do not see this in that documentation (something like warning we use a pool of 4 connections by default so do not use more than 4 nested transactions). Actually sequelize could also detect this and just throw some error instead of letting users to fumble what might be wrong. What if the user is not under direct control like they need to call some external package function which already uses 4 nested transactions with nestMode: TransactionNestMode.separate and they need to call such function inside the transaction (they will just get cryptic error without info that they should increase connections pool size).

[ephys]

All transactions run on their own database connection. That's not a Sequelize implementation detail, that's a pretty hard rule for most if not all SQL databases

All databases also have a limit on the number of concurrent connections, that's also not a Sequelize implementation detail. This is why not limiting the pool size if not a possible solution, nor is having a different pool per nest level, as you can have an infinite number of nest levels.

You're running transactions that are separate from each-other but dependent on each-other. If you have too many of them that are nested, you are going to reach that limit.

If you search for ConnectionAcquireTimeoutError, you should end up on this page: https://sequelize.org/docs/v7/other-topics/connection-pool/#connectionacquiretimeouterror. This page points to a series of probable causes for the issue.

Actually sequelize could also detect this and just throw some error instead of letting users to fumble what might be wrong

If you can see a way to reliably detect this issue, please feel free to open a pull request.

---

The only actual solution to this problem would be to reserve multiple connections before calling the transactions. Something like

// this could actually detect that the number of connections being requested at the same time is bigger than the pool size
sequelize.withConnections(2, connections => {
  // will only run once 2 connections become available at the same time
  return sequelize.transaction({ connection: connections[0] }, () => {
    return sequelize.transaction({ connection: connections[1] }, () => {});
  });
});

[misos1]

@ephys Even if there was not any limit enforced by the database there would always be some limit, it is not possible to have infinite many connections on any computer. I meant that having pool in a way sequelize has is implementation detail. Some more naive implementation could for example create a new connection for each new transaction and when its limit is reached you would get some clear error message instead of deadlock. Some more overthought implementation could track connections and detect deadlock (each connection can have assigned a transaction which is actually doing and there can be a graph of nested transactions) then just report error or to try to create a new connection and if it fails (due to database limit for example, or maybe better some additional sequelize limit for extra connection count) then report error. So this definitely is implementation detail of sequelize that it just deadlocks in such cases.

And withConnections may wait for more time than is necessary.

[ephys]

I meant that having pool in a way sequelize has is implementation detail. Some more naive implementation could for example create a new connection for each new transaction and when its limit is reached you would get some clear error message instead of deadlock

That's already what happens. The code you provided does not hang forever. It errors with a message saying that it could not acquire a connection in the specified time.

Some more overthought implementation could track connections and detect deadlock (each connection can have assigned a transaction which is actually doing and there can be a graph of nested transactions)

I expect that the code required to track this would be very complex, very costly, and it wouldn't even solve the underlying issue, only change the error message.

Something more general that is able to map the inter-dependencies between connections would be great, but the runtime cost would be huge.

Feel free to prove me wrong with a technical solution on how this would work

additional sequelize limit for extra connection count

We're not going to go over the limit specified by the user, that's simply not on the table. It's there for a reason and must be reliable.

So this definitely is implementation detail of sequelize that it just deadlocks in such cases.

Agree to disagree, it would deadlock (or lead to an error, as we do) when you reach the connection limit in any implementation even if you did not use the ORM. Has nothing to do with our implementation details.

And withConnections may wait for more time than is necessary.

If you have way too few connections for your database usage, sure. It would safely solve the actual issue though, not just change the error message.

[misos1]

That's already what happens. The code you provided does not hang forever. It errors with a message saying that it could not acquire a connection in the specified time.

User needs to wait a few minutes for that.

I expect that the code required to track this would be very complex, very costly, and it wouldn't even solve the underlying issue, only change the error message.

Something more general that is able to map the inter-dependencies between connections would be great, but the runtime cost would be huge.

Feel free to prove me wrong with a technical solution on how this would work

Sequelize functions need to detect that they are executed inside a transaction anyway through CLS or by manually passing the transaction. So why not utilize this? Nested transaction then knows that it is executed inside another transaction. This can be used to build the mentioned graph. Now when you have such a graph and also each connection holds info about which transaction it currently executes you can check whether there is a deadlock. For example like in this pseudocode (very rough and simplified, just for illustration):

function sequelize.transaction(callback)
{
	let connection = null;
	for(connection in connections)
	{
		if(connection.free())break;
		if(!cls.transactions_waiting_on_this.contains(connection.transaction))break;
	}
	if(connection == null)throw SomeError;
	await connection.ready();
	current_transaction = ...
	connection.transaction = current_transaction;
	cls.transactions_waiting_on_this.push(current_transaction);
	await callback(current_transaction);
	cls.transactions_waiting_on_this.pop();
}

We're not going to go over the limit specified by the user, that's simply not on the table. It's there for a reason and must be reliable.

That is why there could be 2 limits actually, soft limit like number of connections in pool and hard limit which is never going to be gone over, they can be set to the same value by default.

Agree to disagree, it would deadlock (or lead to an error, as we do) when you reach the connection limit in any implementation even if you did not use the ORM. Has nothing to do with our implementation details.

I meant some other implementation could report an error immediately, not after a few minutes while waiting in a deadlock.

If you have way too few connections for your database usage, sure. It would safely solve the actual issue though, not just change the error message.

Yes, maybe. Just notice that default in sequelize are just 4 connections. And the user needs to manually count how many nested transactions are needed, if that counted number is less then there is deadlock again.